You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "cadonna (via GitHub)" <gi...@apache.org> on 2024/03/22 17:09:55 UTC
[PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
cadonna opened a new pull request, #15581:
URL: https://github.com/apache/kafka/pull/15581
Current logic for auto-committing offsets when partitions are revoked will retry continuously when getting UNKNOWN_TOPIC_OR_PARTITION, leading to the member not completing the revocation in time.
This commit considers error UNKNOWN_TOPIC_OR_PARTITION to be fatal in the context of an auto-commit of offsets before a revocation, even though the error is defined as retriable. This ensures that the revocation can finish in time.
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on PR #15581:
URL: https://github.com/apache/kafka/pull/15581#issuecomment-2018088332
Oh one more detail from the build logs we missed:
```
/home/jenkins/workspace/Kafka_kafka-pr_PR-15581/clients/src/test/java/org/apache/kafka/clients/consumer/internals/MembershipManagerImplTest.java:542: error: cannot find symbol
[2024-03-25T09:49:39.907Z] when(commitRequestManager.maybeAutoCommitSyncNow(anyLong())).thenReturn(commitResult);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on PR #15581:
URL: https://github.com/apache/kafka/pull/15581#issuecomment-2018163251
Sorry for that! I do not know why I am so sloppy with this PR. I am going to fix this now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #15581:
URL: https://github.com/apache/kafka/pull/15581#discussion_r1537639434
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##########
@@ -298,10 +303,13 @@ private void autoCommitSyncNowWithRetries(OffsetCommitRequestState requestAttemp
if (error == null) {
result.complete(null);
} else {
- if (error instanceof RetriableException || isStaleEpochErrorAndValidEpochAvailable(error)) {
+ if ((error instanceof RetriableException || isStaleEpochErrorAndValidEpochAvailable(error))) {
if (error instanceof TimeoutException && requestAttempt.isExpired) {
log.debug("Auto-commit sync timed out and won't be retried anymore");
result.completeExceptionally(error);
+ } else if (error instanceof UnknownTopicOrPartitionException) {
+ log.debug("Auto-commit sync failed because topic or partition were deleted");
Review Comment:
Good idea!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna merged PR #15581:
URL: https://github.com/apache/kafka/pull/15581
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on PR #15581:
URL: https://github.com/apache/kafka/pull/15581#issuecomment-2018802319
Build failures are unrelated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #15581:
URL: https://github.com/apache/kafka/pull/15581#discussion_r1537642692
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##########
@@ -298,10 +303,13 @@ private void autoCommitSyncNowWithRetries(OffsetCommitRequestState requestAttemp
if (error == null) {
result.complete(null);
} else {
- if (error instanceof RetriableException || isStaleEpochErrorAndValidEpochAvailable(error)) {
+ if ((error instanceof RetriableException || isStaleEpochErrorAndValidEpochAvailable(error))) {
if (error instanceof TimeoutException && requestAttempt.isExpired) {
log.debug("Auto-commit sync timed out and won't be retried anymore");
result.completeExceptionally(error);
+ } else if (error instanceof UnknownTopicOrPartitionException) {
+ log.debug("Auto-commit sync failed because topic or partition were deleted");
Review Comment:
I added the `before revocation` to all log messages in this method, since it actually applies to all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #15581:
URL: https://github.com/apache/kafka/pull/15581#discussion_r1537243649
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##########
@@ -272,14 +273,18 @@ private void maybeResetTimerWithBackoff(final CompletableFuture<Map<TopicPartiti
* Commit consumed offsets if auto-commit is enabled, regardless of the auto-commit interval.
* This is used for committing offsets before revoking partitions. This will retry committing
* the latest offsets until the request succeeds, fails with a fatal error, or the timeout
- * expires. Note that this considers {@link Errors#STALE_MEMBER_EPOCH} as a retriable error,
+ * expires. Note that:
+ * - Considers {@link Errors#STALE_MEMBER_EPOCH} as a retriable error,
* and will retry it including the latest member ID and epoch received from the broker.
+ * - Considers {@link Errors#UNKNOWN_TOPIC_OR_PARTITION} as a fatal error, and will not retry
+ * it although the error extends RetrieableException. The reason is that if a topic or partition
Review Comment:
RetriableException :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #15581:
URL: https://github.com/apache/kafka/pull/15581#discussion_r1537638864
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##########
@@ -298,10 +303,13 @@ private void autoCommitSyncNowWithRetries(OffsetCommitRequestState requestAttemp
if (error == null) {
result.complete(null);
} else {
- if (error instanceof RetriableException || isStaleEpochErrorAndValidEpochAvailable(error)) {
+ if ((error instanceof RetriableException || isStaleEpochErrorAndValidEpochAvailable(error))) {
Review Comment:
Good catch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #15581:
URL: https://github.com/apache/kafka/pull/15581#discussion_r1537629081
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##########
@@ -298,10 +303,13 @@ private void autoCommitSyncNowWithRetries(OffsetCommitRequestState requestAttemp
if (error == null) {
result.complete(null);
} else {
- if (error instanceof RetriableException || isStaleEpochErrorAndValidEpochAvailable(error)) {
+ if ((error instanceof RetriableException || isStaleEpochErrorAndValidEpochAvailable(error))) {
Review Comment:
this is failing the build actually
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on PR #15581:
URL: https://github.com/apache/kafka/pull/15581#issuecomment-2015538043
@lianetm Could you please have a look at this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #15581:
URL: https://github.com/apache/kafka/pull/15581#discussion_r1537618245
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##########
@@ -298,10 +303,13 @@ private void autoCommitSyncNowWithRetries(OffsetCommitRequestState requestAttemp
if (error == null) {
result.complete(null);
} else {
- if (error instanceof RetriableException || isStaleEpochErrorAndValidEpochAvailable(error)) {
+ if ((error instanceof RetriableException || isStaleEpochErrorAndValidEpochAvailable(error))) {
if (error instanceof TimeoutException && requestAttempt.isExpired) {
log.debug("Auto-commit sync timed out and won't be retried anymore");
result.completeExceptionally(error);
+ } else if (error instanceof UnknownTopicOrPartitionException) {
+ log.debug("Auto-commit sync failed because topic or partition were deleted");
Review Comment:
nit: would probably make the log clearer to all if we add the before revocation part `Auto-commit sync **before revocation** failed` (just like you did for the func name, like it)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on PR #15581:
URL: https://github.com/apache/kafka/pull/15581#issuecomment-2018046840
Thanks for the changes @cadonna , this will have a great impact I expect. Just left some minor comments, otherwise LGTM once the build passes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #15581:
URL: https://github.com/apache/kafka/pull/15581#discussion_r1537624363
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##########
@@ -298,10 +303,13 @@ private void autoCommitSyncNowWithRetries(OffsetCommitRequestState requestAttemp
if (error == null) {
result.complete(null);
} else {
- if (error instanceof RetriableException || isStaleEpochErrorAndValidEpochAvailable(error)) {
+ if ((error instanceof RetriableException || isStaleEpochErrorAndValidEpochAvailable(error))) {
Review Comment:
do we need these extra parenthesis here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #15581:
URL: https://github.com/apache/kafka/pull/15581#discussion_r1537313537
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##########
@@ -272,14 +273,18 @@ private void maybeResetTimerWithBackoff(final CompletableFuture<Map<TopicPartiti
* Commit consumed offsets if auto-commit is enabled, regardless of the auto-commit interval.
* This is used for committing offsets before revoking partitions. This will retry committing
* the latest offsets until the request succeeds, fails with a fatal error, or the timeout
- * expires. Note that this considers {@link Errors#STALE_MEMBER_EPOCH} as a retriable error,
+ * expires. Note that:
+ * - Considers {@link Errors#STALE_MEMBER_EPOCH} as a retriable error,
* and will retry it including the latest member ID and epoch received from the broker.
+ * - Considers {@link Errors#UNKNOWN_TOPIC_OR_PARTITION} as a fatal error, and will not retry
+ * it although the error extends RetrieableException. The reason is that if a topic or partition
Review Comment:
Thanks! Fixed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org