-
Notifications
You must be signed in to change notification settings - Fork 467
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
Allow number of LeaseRenewer threads to be configured #171
Comments
I'd like to increase the number of streams consumed by my app to around 20, but this means I'll have 400 LeaseRenewer threads 😬 This can't be necessary or useful. |
It's not necessary, but an artifact of the original expectations of the KCL. The original thought was each KCL application would process one stream which would end up with a small number of LeaseRenewer threads. With the move to consuming multiple streams within a single application many of the thread pools should probably be shared. I think the easiest way to solve this would be to allow the application to either provide the thread pool to use, or provide a factory interface that allows the application to decide which thread pool should be used. Here is the code where this issue occurs. Allowing the application to pass in a thread pool, or factory would require changes to the KinesisClientLibConfiguration, and the Worker. |
@pfifer I think you've linked to the lease coordinator thread pool but the bigger problem is the lease renewal thread pool. The former has 2 threads, the latter has 20. So based on your advice, I think if we make this method public: private static ExecutorService getLeaseRenewalExecutorService(int maximumPoolSize) { amazon-kinesis-client/src/main/java/com/amazonaws/services/kinesis/leases/impl/LeaseCoordinator.java Line 368 in 8d339bd
so that people have a way of creating this kind of pool, then add an additional constructor that is similar to this one: public LeaseCoordinator(ILeaseManager<T> leaseManager,
String workerIdentifier,
long leaseDurationMillis,
long epsilonMillis,
int maxLeasesForWorker,
int maxLeasesToStealAtOneTime,
IMetricsFactory metricsFactory) { amazon-kinesis-client/src/main/java/com/amazonaws/services/kinesis/leases/impl/LeaseCoordinator.java Line 131 in 8d339bd
but also accepts a Alternatively, instead of allowing the thread pool to be passed around, we could simply allow the number of threads to come from the KinesisClientLibConfiguration. This is simpler but I agree it's also less flexible. As an even simpler first step, would you be interested in changing static final int MAX_LEASE_RENEWAL_THREAD_COUNT = 20; to static final int MAX_LEASE_RENEWAL_THREAD_COUNT = 10; ? This would drastically reduce the problem for me. |
It seems my tracing of the code path was quite right. Looking at the code it's the result of PR #135. The previous code was actually the worst of both worlds in that it set the maximum, and apparently allowed quick timeouts. It seems the best behavior would be to return to a cached thread pool, but with a minimum that matches the workloads for most workers. This reduce the required threads for small number of leases, while still allowing large number of leases to work normally. |
@pfifer So from your comment I take the following:
I also think we should also consider the following: set core size to be e.g. 1 and max core size to |
I'm looking to revert back to CachedThreadPool, but with some changes to improve the behavior.
|
* Execute graceful shutdown on its own thread * PR awslabs#191 * Issue awslabs#167 * Added support for controlling the size of the lease renewer thread pool * PR awslabs#177 * Issue awslabs#171 * Require Java 8 and later Java 8 is now required for versions 1.8.0 of the amazon-kinesis-client and later. * PR awslabs#176
I have an application that consumes multiple DynamoDB streams - eleven currently. I use a single worker on each stream but each worker creates a LeaseCoordinator with twenty LeaseRenewer threads. So I currently have 220 LeaseRenewer threads.
This seems excessive but there's currently no way to reduce the number of LeaseRenewer threads that I can see.
Would it be acceptable for me to create a PR that reduced the number of LeaseRenewer threads created by default, or simply add a configuration option to allow this number to be reduced?
The text was updated successfully, but these errors were encountered: