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

[FIX_KAFKA_EXECUTOR] fix executor service #3033

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Conversation

elguardian
Copy link
Member

The current approach behaves like monothread.

Copy link

Copy link
Contributor

@martinweiler martinweiler left a comment

Choose a reason for hiding this comment

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

This works properly, thanks @elguardian

Copy link
Member

@porcelli porcelli left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@fjtirado fjtirado left a comment

Choose a reason for hiding this comment

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

I do not think is a good idea to create a fixed number of thread.
According to ThreadPoolExecutor documentation, new threads will be created only if needed. And removed once they are idle for a while.
With this change, we might have a number of threads forever idle when there is not event burst (also the name of the property will be inaccurate)
Lets discuss it a bit to find out why the pool is not working as expected right now, I need to understand that.

@fjtirado
Copy link
Contributor

fjtirado commented Mar 4, 2024

Ok, I misundertood the doc (bold is mine)
Unbounded queues. Using an unbounded queue (for example a LinkedBlockingQueue without a predefined capacity) will cause new tasks to wait in the queue when all corePoolSize threads are busy. Thus, no more than corePoolSize threads will ever be created. (And the value of the maximumPoolSize therefore doesn't have any effect.) This may be appropriate when each task is completely independent of others, so tasks cannot affect each others execution; for example, in a web page server. While this style of queuing can be useful in smoothing out transient bursts of requests, it admits the possibility of unbounded work queue growth when commands continue to arrive on average faster than they can be processed.
I believed it will create threads till maximum is reached and then use the unbounded queue (when the maximun numbers of thread), but its unfortunately the other way around (and that is not configurable, apparently). Fixed number of core threads are a indeed the easiest solution (the new code is equivalent to

           new ThreadPoolExecutorInteger.getInteger(KAFKA_EXTENSION_PREFIX + "maxNotifyThreads", 10), Integer.getInteger(KAFKA_EXTENSION_PREFIX + "maxNotifyThreads", 10),
                                60L,
                                TimeUnit.SECONDS, new LinkedBlockingQueue<>()

),
so lets go for it

And together with
By default, even core threads are initially created and started only when new tasks arrive
makes the property name still accurate (the only caveat if that, once created, the thread will remain there forever, but its a good tradeoff and definetely fixes a bug provoked by my misreading of the doc, my apologies for that)

@fjtirado fjtirado requested review from fjtirado and removed request for fjtirado March 4, 2024 10:30
@fjtirado fjtirado merged commit 106ca10 into main Mar 4, 2024
2 of 4 checks passed
@fjtirado fjtirado added backport-7.67.x Generate backport PR for 7.67.x branch backport-7.67.x-blue Generate backport PR for 7.67.x-blue branch labels Mar 4, 2024
github-actions bot pushed a commit that referenced this pull request Mar 4, 2024
github-actions bot pushed a commit that referenced this pull request Mar 4, 2024
@mareknovotny mareknovotny deleted the fix_kafka_executor branch March 26, 2024 09:14
mareknovotny pushed a commit that referenced this pull request Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-7.67.x Generate backport PR for 7.67.x branch backport-7.67.x-blue Generate backport PR for 7.67.x-blue branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants