lianetm commented on code in PR #23348:
URL: https://github.com/apache/kafka/pull/23348#discussion_r3937740607
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractHeartbeatRequestManager.java:
##########
@@ -259,13 +259,16 @@ public long maximumTimeToWait(long currentTimeMs) {
if (pollTimer.isExpired()) {
return 0L;
}
- // KAFKA-20253: mirror the guard in poll(). A heartbeat is only sent
when the coordinator is known
- // and the member is in a state that heartbeats. When the coordinator
is unavailable (e.g. after a
- // re-authentication failure) or the member should skip heartbeats
(FATAL/FENCED/STALE/UNSUBSCRIBED),
- // poll() returns EMPTY, so falling through to the timer-based
branches below would return 0 (the
- // heartbeat timer is left permanently expired) and busy-spin both the
application and network threads.
+ // Mirror the guard in poll(). A heartbeat is only sent when the
coordinator is known and the
+ // member is in a state that heartbeats. When the coordinator is
unavailable (e.g. after a
+ // re-authentication failure, or while bootstrap DNS resolution is
still in progress) or the
+ // member should skip heartbeats (FATAL/FENCED/STALE/UNSUBSCRIBED),
poll() returns EMPTY, so
+ // falling through to the timer-based branches below would return 0
(the heartbeat timer is left
+ // permanently expired) and busy-spin both the application and network
threads. Wait a retry
+ // backoff rather than the heartbeat interval, because the interval is
zero until the first
+ // heartbeat response is received, which would also busy-spin.
if (coordinatorRequestManager.coordinator().isEmpty() ||
membershipManager().shouldSkipHeartbeat()) {
- return heartbeatRequestState.heartbeatIntervalMs();
+ return heartbeatRequestState.retryBackoffMs();
Review Comment:
Thinking about the case of shouldSkipHB.
That's the case of member:
- UNSUBCRIBED (already handled above btw, and by returning max_value, which
makes sense, we're not waiting for any action on the HBMgr, so can let the app
thread block for the remaining poll timeout).
- FATAL/STALE/FENCED -> these would now return retryBackoff, would that be
correct?
- Fatal should probably be just as unsubscribed (max value), no need to
have the app thread responsive to the req managers any sooner than the poll
timeout (it's a final/unrecoverable state anyways)
- STALE/FENCED -> in those we just need action from the app thread
really (poll to rejoin after stale, and run callbacks if fenced). Do we need to
return a maxTime here to lower the app thread remaining poll timeout?
--
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]
