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(consumer): add retry for offset commit #2040

Closed
wants to merge 2 commits into from

Conversation

HobbyBear
Copy link

@HobbyBear HobbyBear commented Sep 22, 2021

Fixes #1562 #1758
The reason for the timeout when submitting the offset is likely to be the rebalance caused by the server restart or expansion and contraction, and a jittery retry strategy needs to be added.

@HobbyBear HobbyBear requested a review from bai as a code owner September 22, 2021 18:06
@ghost ghost added the cla-needed label Sep 22, 2021
@HobbyBear HobbyBear changed the title feat:add retry for offset commit fix:bugFix add retry for offset commit Sep 22, 2021
@ghost ghost removed the cla-needed label Sep 22, 2021
@bai bai requested a review from dnwe September 24, 2021 04:55
@dnwe
Copy link
Collaborator

dnwe commented Oct 13, 2021

@HobbyBear thanks for putting together these changes. They look reasonable to me, but I have a couple of questions:

  1. Please could you add some unittest coverage for the new behaviour?
  2. As an OffsetCommitRequest for a consumer group can contain offsets for multiple topic+partition combinations and the returned errors are similarly per topic+partition, I'm assuming that it is also possible for the OffsetCommitResponse to contain partial failures amongst some successes. In this code path we'll retry the OffsetCommitRequest as a whole even though some offsets may have been committed successfully, do we need to be filtering those out before the retry or is the broker happy to accept repeated commits for those? I wonder what the Java client does

@bai
Copy link
Contributor

bai commented Jan 21, 2022

@HobbyBear 👋🏼 Friendly bump 🙏🏼 Could you please take a look at the comment above? I'd love to get this one shipped.

@HobbyBear
Copy link
Author

HobbyBear commented Jan 26, 2022

@HobbyBear 👋🏼 Friendly bump 🙏🏼 Could you please take a look at the comment above? I'd love to get this one shipped.

Sorry, I've been a little busy recently. I'm still researching how the java client does retry, and it may not be so fast to get the results.

@JKeita
Copy link

JKeita commented Jul 26, 2023

Has this problem not been solved yet?

@dnwe dnwe changed the title fix:bugFix add retry for offset commit fix(consumer): add retry for offset commit Jul 26, 2023
@dnwe dnwe added the needs-investigation Issues that require followup from maintainers label Aug 24, 2023
Copy link

Thank you for your contribution! However, this pull request has not had any activity in the past 90 days and will be closed in 30 days if no updates occur.
If you believe the changes are still valid then please verify your branch has no conflicts with main and rebase if needed. If you are awaiting a (re-)review then please let us know.

@github-actions github-actions bot added the stale Issues and pull requests without any recent activity label Nov 23, 2023
@github-actions github-actions bot closed this Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-investigation Issues that require followup from maintainers stale Issues and pull requests without any recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected user-specified time limit error
4 participants