lianetm commented on code in PR #16031:
URL: https://github.com/apache/kafka/pull/16031#discussion_r1626534240
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/NetworkClientDelegate.java:
##########
@@ -235,7 +235,6 @@ public void addAll(final List<UnsentRequest> requests) {
public void add(final UnsentRequest r) {
Objects.requireNonNull(r);
- r.setTimer(this.time, this.requestTimeoutMs);
Review Comment:
uhm well there is a difference: before these changes, the timeout was
defined at the `NetworkClientDelegate` level and not per request, which made
sense I would say, since it's the same `REQUEST_TIMEOUT_MS_CONFIG` that needs
to be applied to all UnsentRequests, so it was applied here on this ln 238 when
any request was added.
Now we're saying that each UnsentRequest may have a diff timeout, but I see
we end up with each manager taking that same REQUEST_TIMEOUT_MS_CONFIG config
and passing it to each request.
Outcome is the same, but actually the original version of applying the
REQUEST_TIMEOUT_MS_CONFIG to all requests in the network layer seemed simpler I
would say (unless I'm missing a gain of this option of having the
REQUEST_TIMEOUT_MS_CONFIG at the managers level).
--
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]
