[GitHub] [kafka] ijuma commented on a change in pull request #10157: MINOR: Raft request thread should discover api versions

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

ijuma commented on a change in pull request #10157:
URL: https://github.com/apache/kafka/pull/10157#discussion_r579694847



##########
File path: core/src/main/scala/kafka/raft/KafkaNetworkChannel.scala
##########
@@ -124,8 +124,14 @@ class KafkaNetworkChannel(
     }
 
     def onComplete(clientResponse: ClientResponse): Unit = {
-      val response = if (clientResponse.authenticationException != null) {
-        errorResponse(request.data, Errors.CLUSTER_AUTHORIZATION_FAILED)
+      val response = if (clientResponse.versionMismatch != null) {
+        errorResponse(request.data, Errors.UNSUPPORTED_VERSION)
+      } else if (clientResponse.authenticationException != null) {
+        // For now we treat authentication errors as retriable. We use the
+        // `NETWORK_EXCEPTION` error code for lack of a good alternative.
+        // Note that `BrokerToControllerChannelManager` will still log the
+        // authentication errors so that users have a chance to fix the problem.
+        errorResponse(request.data, Errors.NETWORK_EXCEPTION)

Review comment:
       Why do we treat authentication problems as retriable?

##########
File path: core/src/main/scala/kafka/server/AlterIsrManager.scala
##########
@@ -165,9 +165,19 @@ class DefaultAlterIsrManager(
       new ControllerRequestCompletionHandler {
         override def onComplete(response: ClientResponse): Unit = {
           debug(s"Received AlterIsr response $response")
-          val body = response.responseBody().asInstanceOf[AlterIsrResponse]
           val error = try {
-            handleAlterIsrResponse(body, message.brokerEpoch, inflightAlterIsrItems)
+            if (response.authenticationException != null) {
+              // For now we treat authentication errors as retriable. We use the
+              // `NETWORK_EXCEPTION` error code for lack of a good alternative.
+              // Note that `BrokerToControllerChannelManager` will still log the
+              // authentication errors so that users have a chance to fix the problem.
+              Errors.NETWORK_EXCEPTION
+            } else if (response.versionMismatch != null) {

Review comment:
       Seems like it's pretty common t check for these two fields. Could we have an enum to represent the two errors and then have a function from the enum the Error? Seems a bit easier to use and less error prone.




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