-
Notifications
You must be signed in to change notification settings - Fork 13
Conversation
…feature/Issue28-TestKPLProperties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple small comments, otherwise, looks good! 👍
kplLibConfiguration.getClass.getDeclaredField(configKey.head.toLower + configKey.tail) | ||
field.setAccessible(true) | ||
|
||
println(s"${field.getName}=${field.get(kplLibConfiguration)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
println
//Start with the setters to prevent picking up all the unrelated private fields, stripping the "set" | ||
val configKeys = kplLibConfiguration.getClass.getDeclaredMethods | ||
.filter(_.getName.startsWith("set")) | ||
.map(_.getName.drop(3)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeesh, this seems tricky. We should add another issue to find a way to work with this, without the reflection if possible :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not possible without reflection. We're introspecting the java class to make sure we're covering it all. The alternative is to manually specify each property in the spec, but that isn't future proof.
I use setters because I only care about stuff we can actually set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's pretty solid, if not pretty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, just a bit ugly, but if it's a hard constraint it'll have to do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scala reflection is even uglier imho :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could just do getFields instead, but then you'd have to account for other private fields (like loggers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a test class so i'm not sure if it's too bad
|
||
val streamName = producerConfig.getString("stream-name") | ||
require(!streamName.isEmpty, | ||
"A valid stream name must be provided to start the Kinesis Producer") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead, reword this as something like Config field "stream-name" missing, a value must be provided!
This way the dev knows the exact field he's missing, and where he's missing it from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes:
#28 and #10
KinesisProducerKPL
toKinesisProducer
- that makes this avery small
breaking change.ThreadPoolSize
andThreadingModel
KPL properties, see Add Thread Pooling as a Threading Policy awslabs/amazon-kinesis-producer#100Raised a PR on the AWS KPL to make this a bit better in the future, it's really frustrating how inconsistent the KPL and KCL are in this regard.