KAFKA-20210: Add group.coordinator.background.threads config#21555
KAFKA-20210: Add group.coordinator.background.threads config#21555dajac merged 8 commits intoapache:trunkfrom
Conversation
Add the group.coordinator.executor.threads config option to control the number of threads in the group coordinator's CoordinatorExecutor thread pool.
| public static final int GROUP_COORDINATOR_NUM_THREADS_DEFAULT = 4; | ||
|
|
||
| public static final String GROUP_COORDINATOR_NUM_EXECUTOR_THREADS_CONFIG = "group.coordinator.executor.threads"; | ||
| public static final String GROUP_COORDINATOR_NUM_EXECUTOR_THREADS_DOC = "The number of executor threads used by the group coordinator."; |
There was a problem hiding this comment.
Should we explain what they are used for? We may also want to extend the doc of the runtime threads.
There was a problem hiding this comment.
I updated the config docs. Let me know what you think!
dajac
left a comment
There was a problem hiding this comment.
Thanks for the patch, @squah-confluent. I left a few more comments for consideration.
| public static final String GROUP_COORDINATOR_NUM_THREADS_DOC = "The number of threads used by the group coordinator for processing requests."; | ||
| public static final int GROUP_COORDINATOR_NUM_THREADS_DEFAULT = 4; | ||
|
|
||
| public static final String GROUP_COORDINATOR_NUM_EXECUTOR_THREADS_CONFIG = "group.coordinator.executor.threads"; |
There was a problem hiding this comment.
Thinking about it, I find the name of the config not very good, mainly because it is not clear why it does based on the name and it is not clear how it differs from group.coordinator.threads too. I wonder whether we should use something like group.coordinator.backgroup.threads. @squah-confluent @dongnuo123 Thoughts?
There was a problem hiding this comment.
Should it be group.coordinator.background.threads?
There was a problem hiding this comment.
I renamed the config.
| public static final String GROUP_COORDINATOR_NUM_EXECUTOR_THREADS_DOC = "The number of executor threads used by the group coordinator for " + | ||
| "updating the list of topics for regex subscriptions and metadata changes and offloaded assignments."; |
There was a problem hiding this comment.
The number of executor threads used by the group coordinator for processing background/asynchronous tasks such as resolving regular expressions and computing assignments.
For the computing assignment part, we could mention the related config too when we add it.
| .withCompression(Compression.of(config.offsetTopicCompressionType()).build()) | ||
| .withAppendLingerMs(config.appendLingerMs()) | ||
| .withExecutorService(Executors.newSingleThreadExecutor()) | ||
| .withExecutorService(Executors.newFixedThreadPool(config.numExecutorThreads())) |
There was a problem hiding this comment.
While we are here, would it be possible to give a proper name to the threads used here? We could follow the name we use for the config so administrators can connect the dots.
There was a problem hiding this comment.
I've named the threads group-coordinator-background-<n>
| .withExecutorService(Executors.newSingleThreadExecutor()) | ||
| .withExecutorService(Executors.newFixedThreadPool( | ||
| config.numBackgroundThreads(), | ||
| runnable -> new KafkaThread( |
There was a problem hiding this comment.
nit: It seems that we can now use Thread.ofPlatform().name("group-coordinator-background-", 0).factory() since Java 21. Does it work here?
There was a problem hiding this comment.
I don't think we can, since our min supported Java version is 17
Line 48 in 3fd6017
There was a problem hiding this comment.
17 is for clients. We use a newer one on the server.
There was a problem hiding this comment.
I thought 11 was for clients? I can't get it to compile locally unless I also bump the minNonClientJavaVersion to 21.
There was a problem hiding this comment.
We can't use Thread.ofPlatform() because the server-side code targets JDK 17, and the client-side code is still on JDK 11.
maybe we could use ThreadUtils.createThreadFactory("group-coordinator-background-%d", false) instead?
There was a problem hiding this comment.
My bad... I was sure that we were already on 21. Using ThreadUtils.createThreadFactory makes sense.
There was a problem hiding this comment.
Thanks, I've switched to ThreadUtils.createThreadFactory.
| public static final String GROUP_COORDINATOR_NUM_BACKGROUND_THREADS_DOC = "The number of background threads used by the group coordinator for " + | ||
| "updating the list of topics for regex subscriptions and metadata changes and offloaded assignments."; |
There was a problem hiding this comment.
nit: The number of threads used by the group coordinator for processing background tasks (e.g. regular expressions or assignments).? I don't get the metadata changes part in the current one.
There was a problem hiding this comment.
That's a typo! I meant to write "on metadata changes". Updated the description.
| public static final String GROUP_COORDINATOR_NUM_THREADS_DOC = "The number of threads used by the group coordinator for processing requests."; | ||
| public static final int GROUP_COORDINATOR_NUM_THREADS_DEFAULT = 4; | ||
|
|
||
| public static final String GROUP_COORDINATOR_NUM_BACKGROUND_THREADS_CONFIG = "group.coordinator.background.threads"; |
There was a problem hiding this comment.
Should we add this new config to upgrade.md?
There was a problem hiding this comment.
I am +1 for adding something but I would do it for the KIP, not for the config. Hence, I suggest to defer it to another PR. @squah-confluent Could you please file a ticket so we don't forget about it.
There was a problem hiding this comment.
This reverts commit 5c72195.
FrankYang0529
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the fix.
…21555) Add the group.coordinator.background.threads config option to control the number of threads in the group coordinator's CoordinatorExecutor thread pool. Reviewers: Dongnuo Lyu <dlyu@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>, PoAn Yang <payang@apache.org>, David Jacot <djacot@confluent.io>
Add the group.coordinator.background.threads config option to control
the number of threads in the group coordinator's CoordinatorExecutor
thread pool.
Reviewers: Dongnuo Lyu dlyu@confluent.io, Chia-Ping Tsai
chia7712@gmail.com, PoAn Yang payang@apache.org, David Jacot
djacot@confluent.io