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 #1397 #1505

Merged
merged 2 commits into from
Nov 6, 2017
Merged

Fix #1397 #1505

merged 2 commits into from
Nov 6, 2017

Conversation

edenhill
Copy link
Contributor

@edenhill edenhill commented Nov 6, 2017

This is two fixes for the same thing, but they are valuable each on their own.

With a zero rkb_blocking_max_ms the ua_idle() function would
not serve broker queues and IOs resulting in the broker thread practically
hanging and busy-looping.

This fix makes sure we serve it at least once per spin.
The previous fix makes sure rkb_blocking_max_ms is not 0.
@edenhill edenhill added the bug label Nov 6, 2017
@edenhill edenhill requested a review from mhowlett November 6, 2017 20:21
* But honour the lower on-termination blocking time. */
if (rd_kafka_terminating(rkb->rkb_rk))
rkb->rkb_blocking_max_ms = 1;
else
Copy link
Contributor

Choose a reason for hiding this comment

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

my understanding is limited, but this looks good to me (resetting back to what it would be on a new connection). I wonder if you need to explicitly set this to 1 in the termination case though, but doesn't hurt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worst case the thread will block on io-wait for blocking_max_ms (def 1000ms), this slows down termination, so we minimize the blocking time when we know that we are terminating.

@mhowlett
Copy link
Contributor

mhowlett commented Nov 6, 2017

looks good to the extent of my knowledge. I don't completely understand the reasoning behind the change in rd_kafka_broker_ua_idle.

@edenhill edenhill merged commit cd1f612 into master Nov 6, 2017
@edenhill edenhill deleted the fix_1397 branch November 15, 2017 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants