Skip to content
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

setThreadingModel() should take String, and setThreadPoolSize() should take long #124

Open
bowenli86 opened this issue Aug 24, 2017 · 3 comments

Comments

@bowenli86
Copy link

Currently, fromProperties(Properties props) can only recognize boolean, string, and long. While the signature of thread-related methods are setThreadingModel(enum) and setThreadPoolSize(int). Thus, these properties cannot be set via fromProperties(Properties props).

You should change the signature to setThreadingModel(String) and setThreadPoolSize(long).

@bowenli86
Copy link
Author

Alternatively, for ThreadPoolSize, add the following code to fromProperties()

else if (type == int.class) {
  method.invoke(config, value);
}

@markglh
Copy link
Contributor

markglh commented Sep 8, 2017

I think this should go further, the KCL is much better at supporting configuration:
https://github.com/awslabs/amazon-kinesis-client/blob/master/src/main/java/com/amazonaws/services/kinesis/clientlibrary/config/KinesisClientLibConfigurator.java#L112

You can set not only primitive types, but the credential providers etc, too. This should be abstracted and shared between the libraries. It makes no sense to have such inconsistency between the two.

A quick (and correct?) fix would be to add support for Int here like you suggested:
https://github.com/awslabs/amazon-kinesis-producer/blob/master/java/amazon-kinesis-producer/src/main/java/com/amazonaws/services/kinesis/producer/KinesisProducerConfiguration.java#L197

And add an additional setter for the ThreadingModel which accepts Strings:

public KinesisProducerConfiguration setThreadingModel(String threadingModel) {
        setThreadingModel(ThreadingModel.valueOf(stringValue))
}

The inconsistent design of these two libraries can be pretty frustrating at times.

markglh added a commit to markglh/amazon-kinesis-producer that referenced this issue Sep 8, 2017
@markglh
Copy link
Contributor

markglh commented Sep 8, 2017

Fixed here: #134

pfifer pushed a commit that referenced this issue Nov 13, 2017
* Fix issue #124 - threading properties

* addressed comments, updated gitignore

* fixed typo

* revert formatting changes
pfifer added a commit to pfifer/amazon-kinesis-producer that referenced this issue Nov 14, 2017
* 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
pfifer added a commit that referenced this issue Nov 14, 2017
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants