Prevent offsets from being committed manually when a rebalance is in progress and partitions are owned by consumer#4089
Conversation
…n a rebalance is in progress and partitions are owned by consumer
|
|
||
| /* Don't attempt auto commit when rebalancing or initializing since | ||
| * the rkcg_generation_id is most likely in flux. */ | ||
| if (rkcg->rkcg_subscription && |
There was a problem hiding this comment.
This is not thread safe (this is on the application thread, but rkcg is local to the rdkafka main thread).
Check should probably be performed in rd_kafka_cgrp_offsets_commit().
But we need to make sure that it is ok to commit during the phase of the rebalance when the rebalance callback/event is triggered.
There was a problem hiding this comment.
Fixed, I also removed the same check from rd_kafka_cgrp_offset_commit_tmr_cb since the function will also call rd_kafka_cgrp_offsets_commit. I am not sure if this is the ideal behavior for auto commit though as we would no longer "silently" skip committing offsets...
Magnus Edenhill (edenhill)
left a comment
Magnus Edenhill (edenhill)
left a comment
There was a problem hiding this comment.
Also make sure to run make style-fix (on linux, since it si broken on osx)
| @@ -0,0 +1,140 @@ | |||
| /* | |||
There was a problem hiding this comment.
Bravo on test!
|
Roxane (@roxelo) whats the status of this PR? Any help needed? |
|
William Morgan (@wmorgan6796) Some of the tests failed and I haven't had a chance to look at it further. If you have some time to investigate, another set of eyes would help |
I believe I found the reason for the test failure. We fundamentally change how offset committing occurs by short circuiting even sending the OffsetCommitRequest to the brokers. When we send the request to the brokers, we will get an I think the correct thing to do is allow for the error returned to change, but ultimately would leave it up to the maintainers to decide is ok. In the mean time I'll bring this branch up to date with the latest librdkafka |
|
Milind L (@milindl) Emanuele Sabellico (@emasab) regarding this, what are your thoughts? |
|
I am closing this PR since it's out of date and William Morgan (@wmorgan6796) opened #4220 to fix the issue |
3 participants
See #4059
When using cooperative sticky, attempting to commit offsets during a rebalance can trigger a follow up rebalance.
One way of reducing the risk of this happening, is to add the same check that is included in the auto commit function in the manual commit method