lianetm commented on code in PR #16031:
URL: https://github.com/apache/kafka/pull/16031#discussion_r1624620726
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##########
@@ -432,7 +436,7 @@ private void commitSyncWithRetries(OffsetCommitRequestState
requestAttempt,
result.complete(null);
} else {
if (error instanceof RetriableException) {
- if (error instanceof TimeoutException &&
requestAttempt.isExpired) {
+ if (error instanceof TimeoutException &&
requestAttempt.isExpired()) {
Review Comment:
If I remember right, that was needed at some point to avoid retrying on
[this](https://github.com/apache/kafka/blob/a68a1cce824a8346509d5194e0e43a3cb36ba09a/clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java#L838)
`TimeoutException` internally generated by the manager when expiring requests
on poll (vs retrying on the request `TimeoutException` received as a response
if the api level timeout was still not expired). But with this PR the manager
does not do that anymore, so agree we should check isExpired and fail, no
matter the exception.
Note that there more to this. Before, the manager was internally throwing a
TimeoutException, so at this point we could simply completeExceptionally with
the same error (ln 441). But this also changes now. Similar to how the
`TopicMetadataManager` does, if at this point we see that the request
isExpired, I guess we need to throw a new TimeoutException (not the last known
error, which is what is thrown now in ln 441)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
