You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "squah-confluent (via GitHub)" <gi...@apache.org> on 2024/08/07 13:31:25 UTC
[PR] KAFKA-17267: Don't return REQUEST_TIMED_OUT for OFFSET_FETCHes (#8) [kafka]
squah-confluent opened a new pull request, #16825:
URL: https://github.com/apache/kafka/pull/16825
When handling an OFFSET_FETCH request requiring stable offsets, the new
group coordinator may encounter a timeout under some circumstances, such
as a zombie coordinator or a lagging __consumer_offsets replica that has
not yet dropped out of the ISR. Existing and older clients do not expect
the REQUEST_TIMED_OUT error code won't retry, so remap it to
NOT_COORDINATOR to trigger a coordinator lookup and retry.
### 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-17267: Don't return REQUEST_TIMED_OUT for OFFSET_FETCHes [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac merged PR #16825:
URL: https://github.com/apache/kafka/pull/16825
--
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-17267: Don't return REQUEST_TIMED_OUT for OFFSET_FETCHes [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16825:
URL: https://github.com/apache/kafka/pull/16825#discussion_r1707020093
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorService.java:
##########
@@ -1176,4 +1176,34 @@ private <IN, OUT> OUT handleOperationException(
return handler.apply(apiError.error(), apiError.message());
}
}
+
+ /**
+ * This is the handler used by offset fetch operations to convert errors to coordinator errors.
+ *
+ * @param request The OffsetFetchRequestGroup request.
+ * @param exception The exception to handle.
+ * @return The OffsetFetchRequestGroup response.
+ */
+ private OffsetFetchResponseData.OffsetFetchResponseGroup handleOffsetFetchException(
+ OffsetFetchRequestData.OffsetFetchRequestGroup request,
+ Throwable exception
+ ) {
+ ApiError apiError = ApiError.fromThrowable(exception);
+
+ switch (apiError.error()) {
Review Comment:
I think that we should actually handle all the errors handled by `handleOperationException` too. A write operation could actually return some of those at any time. What do you think?
Handling `UNKNOWN_SERVER_ERROR` while we are here will also be a nice addition.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorService.java:
##########
@@ -1176,4 +1176,34 @@ private <IN, OUT> OUT handleOperationException(
return handler.apply(apiError.error(), apiError.message());
}
}
+
+ /**
+ * This is the handler used by offset fetch operations to convert errors to coordinator errors.
+ *
+ * @param request The OffsetFetchRequestGroup request.
+ * @param exception The exception to handle.
+ * @return The OffsetFetchRequestGroup response.
+ */
+ private OffsetFetchResponseData.OffsetFetchResponseGroup handleOffsetFetchException(
+ OffsetFetchRequestData.OffsetFetchRequestGroup request,
+ Throwable exception
+ ) {
+ ApiError apiError = ApiError.fromThrowable(exception);
+
+ switch (apiError.error()) {
+ case REQUEST_TIMED_OUT:
+ // Remap REQUEST_TIMED_OUT to NOT_COORDINATOR, otherwise older consumers will not
Review Comment:
nit: Should we add the version too? We could say consumers on version prior to 3.9. I say 3.9 because I think that we should cherry-pick your other PR to 3.9 branch.
--
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-17267: Don't return REQUEST_TIMED_OUT for OFFSET_FETCHes [kafka]
Posted by "squah-confluent (via GitHub)" <gi...@apache.org>.
squah-confluent commented on code in PR #16825:
URL: https://github.com/apache/kafka/pull/16825#discussion_r1709410881
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorService.java:
##########
@@ -1176,4 +1176,34 @@ private <IN, OUT> OUT handleOperationException(
return handler.apply(apiError.error(), apiError.message());
}
}
+
+ /**
+ * This is the handler used by offset fetch operations to convert errors to coordinator errors.
+ *
+ * @param request The OffsetFetchRequestGroup request.
+ * @param exception The exception to handle.
+ * @return The OffsetFetchRequestGroup response.
+ */
+ private OffsetFetchResponseData.OffsetFetchResponseGroup handleOffsetFetchException(
+ OffsetFetchRequestData.OffsetFetchRequestGroup request,
+ Throwable exception
+ ) {
+ ApiError apiError = ApiError.fromThrowable(exception);
+
+ switch (apiError.error()) {
+ case REQUEST_TIMED_OUT:
+ // Remap REQUEST_TIMED_OUT to NOT_COORDINATOR, otherwise older consumers will not
Review Comment:
Yes, good idea. I've updated the comment to mention version 3.9.
--
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-17267: Don't return REQUEST_TIMED_OUT for OFFSET_FETCHes [kafka]
Posted by "squah-confluent (via GitHub)" <gi...@apache.org>.
squah-confluent commented on code in PR #16825:
URL: https://github.com/apache/kafka/pull/16825#discussion_r1709411875
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorService.java:
##########
@@ -1176,4 +1176,34 @@ private <IN, OUT> OUT handleOperationException(
return handler.apply(apiError.error(), apiError.message());
}
}
+
+ /**
+ * This is the handler used by offset fetch operations to convert errors to coordinator errors.
+ *
+ * @param request The OffsetFetchRequestGroup request.
+ * @param exception The exception to handle.
+ * @return The OffsetFetchRequestGroup response.
+ */
+ private OffsetFetchResponseData.OffsetFetchResponseGroup handleOffsetFetchException(
+ OffsetFetchRequestData.OffsetFetchRequestGroup request,
+ Throwable exception
+ ) {
+ ApiError apiError = ApiError.fromThrowable(exception);
+
+ switch (apiError.error()) {
Review Comment:
I've updated the handler to fall through to `handleOperationException`.
--
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