Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
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
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
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
Re: [PR] KAFKA-16224: Do not retry committing if topic or partition deleted [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
