kevin-wu24 commented on code in PR #19668:
URL: https://github.com/apache/kafka/pull/19668#discussion_r2098713276
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -2762,25 +2767,27 @@ private void handleInboundMessage(RaftMessage message,
long currentTimeMs) {
* @param currentTimeMs the current time
* @param destination the node receiving the request
* @param requestSupplier the function that creates the request
+ * @param api the type of request to maybe send
* @return the first element in the pair indicates if the request was
sent; the second element
* in the pair indicates the time to wait before retrying.
*/
private RequestSendResult maybeSendRequest(
long currentTimeMs,
Node destination,
- Supplier<ApiMessage> requestSupplier
+ Supplier<ApiMessage> requestSupplier,
+ ApiKeys api
) {
var requestSent = false;
- if (requestManager.isBackingOff(destination, currentTimeMs)) {
- long remainingBackoffMs =
requestManager.remainingBackoffMs(destination, currentTimeMs);
+ if (requestManager.isBackingOff(destination, currentTimeMs, api)) {
+ long remainingBackoffMs =
requestManager.remainingBackoffMs(destination, currentTimeMs, api);
logger.debug("Connection for {} is backing off for {} ms",
destination, remainingBackoffMs);
return RequestSendResult.of(requestSent, remainingBackoffMs);
}
- if (requestManager.isReady(destination, currentTimeMs)) {
+ if (requestManager.isReady(destination, currentTimeMs, api)) {
Review Comment:
> Is this true when the request is FETCH or FETCH_SNAPSHOT? Don't we need to
make sure that neither FETCH nor FETCH_SNAPSHOT is pending?
By the time we enter this method with a `requestSupplier` that supplies
FETCH or FETCH_SNAPSHOT, we already performed checks to ensure no pending FETCH
or FETCH_SNAPSHOT exists:
- The first of two usages of this method with a supplied FETCH or
FETCH_SNAPSHOT is in `maybeSendFetchOrFetchSnapshot`, which only executes when
neither request is in-flight.
- The other usage is in `maybeSendFetchToAnyBootstrap`, and will not enter
`maybeSendRequest` if there is a pending FETCH or FETCH_SNAPSHOT because the
call to `requestManager.findReadyBootstrapServer` will return an empty optional.
I think these checks in `maybeSendRequest` are redundant for FETCH and
FETCH_SNAPSHOT (but not necessarily for other request types) in the old
implementation too, because the above usages do the same thing when the request
manager can only handle 1 in-flight request.
> I am wondering if this grouping of FETCH and FETCH_SNAPSHOT should be
encoded in the request manager.
I thought we wanted the KafkaRaftClient to verify at most one pending
FETCH/FETCH_SNAPSHOT. Maybe since these two RPCs are a special case, we should
have a separate `maybeSendFetchOrSnapshot`
--
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
