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 slow rebalanceSafeCommits behavior #1358

Merged
merged 2 commits into from
Nov 2, 2024

Conversation

svroonland
Copy link
Collaborator

@svroonland svroonland commented Nov 2, 2024

After consumer 1 is shutdown (using stopConsumption), rebalances happen and partitions from consumer 2 are assigned. These streams are never started, so the finalizer completing completedPromise is never called. Waiting for these to complete takes 3 minutes (default maxRebalanceDuration).

In case that streams were assigned and no record was ever put in their queues, there's no use in waiting for the stream to complete by committing some offset.

After consumer 1 is shutdown (using stopConsumption), rebalances happen and partitions from consumer 2 are assigned. These streams are never started, so the finalizer completing completedPromise is never called. Waiting for these to complete takes 3 minutes (default maxRebalanceDuration).

In case that streams were assigned and no record was ever put in their queues, there's no need to wait for the stream to complete.
@svroonland svroonland marked this pull request as ready for review November 2, 2024 16:53
@svroonland
Copy link
Collaborator Author

@erikvanoosten If this mechanism is okay by you, we can add to it:

  • Logging (debug) the situation that the stream has not pulled any data so we're not awaiting for it to complete
  • Logging (warning) when we're done awaiting a stream to complete but the deadline has passed. Right now it's not clear why the wait takes so long.

Copy link
Collaborator

@erikvanoosten erikvanoosten left a comment

Choose a reason for hiding this comment

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

Smart!

@erikvanoosten
Copy link
Collaborator

erikvanoosten commented Nov 2, 2024

@erikvanoosten If this mechanism is okay by you, we can add to it:

  • Logging (debug) the situation that the stream has not pulled any data so we're not awaiting for it to complete
  • Logging (warning) when we're done awaiting a stream to complete but the deadline has passed. Right now it's not clear why the wait takes so long.

IMHO this PR is already very valuable in itself and I wouldn't await merging. These 2 improvements can be put in follow up PRs.

@svroonland
Copy link
Collaborator Author

Alright let's do that.

@svroonland svroonland merged commit d59e8f6 into master Nov 2, 2024
14 checks passed
@svroonland svroonland deleted the slow-rebalance-safe-commits branch November 2, 2024 17:40
svroonland added a commit that referenced this pull request Nov 10, 2024
)

Follow up of #1358 

See also #1132

---------

Co-authored-by: Erik van Oosten <e.vanoosten@grons.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants