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.
On top of that valid point from @cadonna, I think there's 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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
