Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Feature/issue28 test kpl properties #30

Merged
merged 17 commits into from
Sep 11, 2017

Conversation

markglh
Copy link
Contributor

@markglh markglh commented Sep 8, 2017

Fixes:
#28 and #10

  • Refactored how the properties are loaded to share commonality between the actor and Scala implementations. This simplifies the Scala implementation and makes it more robust. It also allows more control over the underlying KPL properties should someone want to not use the Typesafe config approach.
  • Removed the pointless trait in the Producer and renamed KinesisProducerKPL to KinesisProducer - that makes this a very small breaking change.
  • The Producer properties are now automatically tested reflectively against the underlying KPL implementation, so our tests will fail automatically when new ones are added or existing ones changed. I'll do the same for the Consumer in a different PR.
  • Tidied a few test exceptions and (hopefully) fixed an intermittent test
  • Updated the readme to show the tweaked usage of the Scala implementation.
  • Added ThreadPoolSize and ThreadingModel KPL properties, see Add Thread Pooling as a Threading Policy awslabs/amazon-kinesis-producer#100

Raised 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.

@coveralls
Copy link

coveralls commented Sep 8, 2017

Coverage Status

Coverage increased (+0.09%) to 86.74% when pulling a758a85 on feature/Issue28-TestKPLProperties into fc7fee2 on master.

j-potts
j-potts previously approved these changes Sep 8, 2017
Copy link

@j-potts j-potts left a 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)}")
Copy link

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))
Copy link

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 :/

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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)

Copy link
Contributor

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")
Copy link

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.

@coveralls
Copy link

coveralls commented Sep 8, 2017

Coverage Status

Coverage increased (+0.1%) to 86.777% when pulling 2997479 on feature/Issue28-TestKPLProperties into fc7fee2 on master.

j-potts
j-potts previously approved these changes Sep 8, 2017
Copy link

@j-potts j-potts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

Copy link

@felixt-cake felixt-cake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@coveralls
Copy link

coveralls commented Sep 8, 2017

Coverage Status

Coverage increased (+0.1%) to 86.777% when pulling 9f60a4c on feature/Issue28-TestKPLProperties into fc7fee2 on master.

@coveralls
Copy link

coveralls commented Sep 9, 2017

Coverage Status

Coverage increased (+0.1%) to 86.777% when pulling 6fb2fa5 on feature/Issue28-TestKPLProperties into fc7fee2 on master.

@coveralls
Copy link

coveralls commented Sep 9, 2017

Coverage Status

Coverage increased (+0.1%) to 86.777% when pulling d77b0d1 on feature/Issue28-TestKPLProperties into fc7fee2 on master.

@agaro1121 agaro1121 merged commit 6fe5371 into master Sep 11, 2017
@agaro1121 agaro1121 deleted the feature/Issue28-TestKPLProperties branch February 7, 2018 03:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants