-
Notifications
You must be signed in to change notification settings - Fork 330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issue #124 - threading properties #134
Fix issue #124 - threading properties #134
Conversation
The formatting changes are in line with java standards - but I'm happy to undo them if it's an issue. |
* @param threadingModel the String representation of the threading model to use. | ||
* @return this configuration object | ||
*/ | ||
public KinesisProducerConfiguration setThreadingModel(String threadingModel) { |
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.
can you add valid strings to both this java doc and https://github.com/awslabs/amazon-kinesis-producer/blob/master/java/amazon-kinesis-producer-sample/default_config.properties for reference
Properties p = new Properties(); | ||
p.setProperty("xcdfndetnedtne5tje45", "Sfbsfrne34534"); | ||
KinesisProducerConfiguration.fromPropertiesFile(writeFile(p)); | ||
// should not throw exception | ||
} | ||
|
||
@Test | ||
public void setThreadingModelFromProperties() { |
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.
can you also test PER_REQUEST?
Comments addressed @bowenli86 I've also updated the |
LGTM! |
Thanks @bowenli86 - how do we get this one merged/released? :) |
@markglh ask an AWS guy? I'm not from Amazon, so I don't have access |
Ah I thought you might have been working with/for them! I'm guessing @pfifer is the one to prod? :D |
Thanks for providing, but a small request: Can you back out the whitespace changes to the KinesisProducerConfiguration.java? It makes reviewing the diff more difficult. Thanks |
aee51f3
to
171f767
Compare
@pfifer no worries, that's all done now! Apologies for the delay, I've been on vacation :) Cheers! |
Please confirm that we can use, modify, copy, and redistribute this contribution. Thanks. |
@pfifer of course: |
@pfifer can you take a look and release a new version after merging this PR? |
Yeah 2 months to merge makes me sad :( |
@markglh You've done a great job on this :) I'm upset about Amazon's speed too. |
Sorry for the delay. I will be merging this soon. |
Hi @pfifer , when can Amzn release KPL 0.10.6 with this fix? |
I'm in the process of testing the release, and my hope is to be done with testing today. |
* Added Windows support The 0.12.x version now supports Windows. The Windows version is currently mastered on the branch `windows`, which will be merged at a later date. The build instructions for Windows are currently out of date, and will be updated at a later date.__ * Issue awslabs#113 * Issue awslabs#74 * Issue awslabs#73 * Removed the libc wrapper The libc wrapper lowered the required version of glibc. The KPL is now built with an older version of libc, which removes the need for the wrapper. * PR awslabs#139 * Set the minimum required version of macOS to 10.9. The KPL is now built against macOS 10.9. * Issue awslabs#117 * PR awslabs#138 * Allow exceptions to bubble to the thread exception handler for Daemon threads. Exceptions that occur on daemon threads will now be allowed to propagate to the thread exception handler. This doesn't provide any additional monitoring or handling of thread death. * PR awslabs#112 * Issue awslabs#111 * Updated `amazon-kinesis-producer-sample` to use the correct properties in its configuration file. * PR awslabs#120 * Issue awslabs#119 * Updated documentation of `AggregationMaxSize` to match actual Kinesis limits. * PR awslabs#133 * Added support for setting `ThreadingModel`, and `ThreadPoolSize` using a properties file. * PR awslabs#134 * Issue awslabs#124 * Extracted `IKinesisProducer` from `KinesisProducer` to allow for easier testing. * PR awslabs#136
* Added Windows support The 0.12.x version now supports Windows. The Windows version is currently mastered on the branch `windows`, which will be merged at a later date. The build instructions for Windows are currently out of date, and will be updated at a later date.__ * Issue #113 * Issue #74 * Issue #73 * Removed the libc wrapper The libc wrapper lowered the required version of glibc. The KPL is now built with an older version of libc, which removes the need for the wrapper. * PR #139 * Set the minimum required version of macOS to 10.9. The KPL is now built against macOS 10.9. * Issue #117 * PR #138 * Allow exceptions to bubble to the thread exception handler for Daemon threads. Exceptions that occur on daemon threads will now be allowed to propagate to the thread exception handler. This doesn't provide any additional monitoring or handling of thread death. * PR #112 * Issue #111 * Updated `amazon-kinesis-producer-sample` to use the correct properties in its configuration file. * PR #120 * Issue #119 * Updated documentation of `AggregationMaxSize` to match actual Kinesis limits. * PR #133 * Added support for setting `ThreadingModel`, and `ThreadPoolSize` using a properties file. * PR #134 * Issue #124 * Extracted `IKinesisProducer` from `KinesisProducer` to allow for easier testing. * PR #136
Fixes issue #124
This is the quick fix imho. The long term fix for the property resolution is to migrate to the KCL approach I described in the issue. This would also then allow support for CredentialProviders too.