-
Notifications
You must be signed in to change notification settings - Fork 139
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
ReactorProcessor executes a user-provided function in a thread from the pc-pool rather than in the provided scheduler #793
Comments
Hi @yevheniisemenov, Thanks for detailed explanation of the issue and PR with a fix. I am not very well versed in usage of Reactor framework so the explanation of the problem is very helpful. Cheers |
@rkolesnev, thanks for merging! I had a follow-up question: when can I expect to see this commit in the next release? Thanks! |
I am actually preparing the release - your PR got included last minute as I thought will rather included it now than wait for next release. |
Sounds great, thank you! |
Fix merged, closing the issue. |
As far as I understand, when I use
ReactorProcessor
, it's expected that the user-provided function will be executed in a scheduler that I provide during initialization. However, it actually executes in a thread from thepc-pool
pool.For example, this code
prints
while the expected (as for me) result should be
This means that with the current implementation, it's possible for users to block threads from the
pc-pool
, which may not be clear or expected behavior.Also, if we dive deeper into the
react
implementation, we can see that it has multiple thread switches: the first by.subscribeOn
and the second by.publishOn
.parallel-consumer/parallel-consumer-reactor/src/main/java/io/confluent/parallelconsumer/reactor/ReactorProcessor.java
Lines 98 to 109 in 1819a8b
However,
.subscribeOn
only works for the reactive part of the user-defined function, which is why the first print from my example is executed in apc-pool
thread.As for
.publishOn
, it switches threads again for thedoOnNext
andonComplete
parts, which seems unnecessary to me - we just spent CPU on a thread switch here.To fix this problem, we need to move
carefullyRun
into the reactive context and removepublishOn
. This should be enough.I can provide a PR later today if you agree with my proposal.
Thanks!
The text was updated successfully, but these errors were encountered: