-
Notifications
You must be signed in to change notification settings - Fork 41
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
Blocking calls in kafta tests #129
Comments
I don't think the test runs on the event loop, so blocking is acceptable. That being said, in this case, Another aspect to consider is that, anyway, the |
@cescoffier Thank you for the detailed response!
Hmmm tbh I'm not an expert on reactive programming, but this
Makes sense :). though is there not a more reactive approach of handling errors in such scenarios? 🤔 |
Do you have the name of the thread running the test? Anyway, if that test must not be blocked, I don't believe sending like this should be allowed (probably not detected, but it can block). |
hm, I would have to rerun it again with the profiler, let me get back to you once I have it. 👍 |
HI! 🙂
Thank you for this project.
As we were going through the source , we found these blocking calls in
AbstractIT
class of theKafka.it
tests:This method is called in another method where a timed await is called immediately after calling the above (sendToTopic) method:
Which got us thinking , if we are waiting anyway, then why use
.block
operator and block the event loop.. Would it not be just fine to subscribe? 🤔 We tried the test cases withsubscribe()
and they still passed..We also evaluated the performance difference when using block vs subscribe (in terms of thread count and heap usage), and the subscribe version seem to outperform in both.
I know this is just test code but considering many users are and will be using it for their project, we might as well set good example by avoiding blocking calls wherever possible in a reactive chain/event loop.
We created a PR with the change in case you deem it fit: #130 :)
The text was updated successfully, but these errors were encountered: