stanislavkozlovski commented on code in PR #15130:
URL: https://github.com/apache/kafka/pull/15130#discussion_r1442732279
##########
clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java:
##########
@@ -158,7 +158,9 @@ public Iterable<NetworkClient.InFlightRequest>
clearAll(String node) {
private Boolean hasExpiredRequest(long now,
Deque<NetworkClient.InFlightRequest> deque) {
for (NetworkClient.InFlightRequest request : deque) {
- if (request.timeElapsedSinceSendMs(now) > request.requestTimeoutMs)
+ // We exclude throttle time here because we want to ensure that we
don't expire requests while
+ // they are throttled. The request timeout should take effect only
after the throttle time has elapsed.
+ if (request.timeElapsedSinceSendMs(now) - request.throttleTimeMs()
> request.requestTimeoutMs)
return true;
Review Comment:
one minor tricky thing here is that we never have a guarantee that we
actually throttled fro `throttleTimeMs` precisely, because:
- `time.milliseconds()` isn't monotonic
- we use `time.milliseconds()` to compute the end time for throttling
But this is equally inconsistent with the other two things here -
`request.timeElapsedSinceSendMs(now)` and `request.requestTimeoutMs` - so I
guess that's fine. Not sure why Kafka doesn't use hiResClockMs here
--
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
