lianetm commented on code in PR #17109:
URL: https://github.com/apache/kafka/pull/17109#discussion_r1747149449
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##########
@@ -753,9 +754,14 @@ public void onResponse(final ClientResponse response) {
"failed with unknown member ID. " +
error.message()));
return;
} else if (error == Errors.STALE_MEMBER_EPOCH) {
+ String lastEpoch = "undefined";
+ if (lastEpochSentOnCommit.isPresent()) {
Review Comment:
with this, we're bringing in here the logic for how STALE_EPOCH may be
retried, which is truly something that we have in the caller func (stale_epoch
is fatal at this response handling level, because it's really a fatal error,
but if the caller is the auto commit before revocation, it does an exception
and retries if isStaleEpochErrorAndValidEpochAvailable).
Also, before this change, we would call `onFailedAttempt` for all errors,
but now we're just calling it for some. That could be risky I would say,
because we don't know what the caller funcs will do with the error (now or in
the future). So it seems conceptually right to record a failed attempt if we
got one, always, like we used to. If the caller retries it, it will have the
accurate info for attempts & backoff. If it doesn't, it will jsut discard a
request with a number of attempts =1 that it's really not used (but it's
accurate, we did attempt once).
What about just applying here the same `hasError` approach you did for the
fetch? That approach seems to me exactly what to want.
--
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]
