[GitHub] [kafka] hachikuji commented on a change in pull request #9564: KAFKA-10667: add timeout for forwarding requests

[GitBox <gi...@apache.org>]

hachikuji commented on a change in pull request #9564:
URL: https://github.com/apache/kafka/pull/9564#discussion_r525428214



##########
File path: clients/src/main/java/org/apache/kafka/clients/RequestCompletionHandler.java
##########
@@ -24,4 +24,11 @@
 
     void onComplete(ClientResponse response);
 
+    /**
+     * Fire when the request transmission hits a fatal exception.
+     *
+     * @param exception the thrown exception
+     */
+    default void onFailure(RuntimeException exception) {

Review comment:
       I am not so sure about adding this to `RequestCompletionHandler`. This interface is mainly serving `NetworkClient`, but there are no changes to `NetworkClient` here.

##########
File path: core/src/main/scala/kafka/server/BrokerToControllerChannelManagerImpl.scala
##########
@@ -178,6 +183,11 @@ class BrokerToControllerRequestThread(networkClient: KafkaClient,
     }
   }
 
+  private def isTimedOut(response: ClientResponse): Boolean = {
+    val requestCreatedTime = response.receivedTimeMs() - response.requestLatencyMs()

Review comment:
       The time we need to track should begin when the request is submitted to the manager. This looks like it is just checking how long an individual request is taking, which the request timeout in `NetworkClient` already takes care of.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org