jolshan commented on code in PR #14774:
URL: https://github.com/apache/kafka/pull/14774#discussion_r1414708109


##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -708,23 +708,40 @@ class KafkaApis(val requestChannel: RequestChannel,
       }
     }
 
-    if (authorizedRequestInfo.isEmpty)
-      sendResponseCallback(Map.empty)
-    else {
-      val internalTopicsAllowed = request.header.clientId == 
AdminUtils.ADMIN_CLIENT_ID
+    val internalTopicsAllowed = request.header.clientId == 
AdminUtils.ADMIN_CLIENT_ID
+    val transactionVerificationEntries = new 
ReplicaManager.TransactionVerificationEntries
 
-      // call the replica manager to append messages to the replicas
+    def postVerificationCallback(newRequestLocal: RequestLocal)

Review Comment:
   The problem I had was that I needed to pass the errors and the verification 
guards into the postVerificationCallback.
   
   The alternative is to create yet another method in ReplicaManager for the 
transaction produce path. I was hoping to avoid many methods with 7+ arguments 
that look similar but if we really want to move out of Kafka Apis, we can do 
that.
   
   I'm not sure I fully agree that the code was good before. I think it is 
better to keep append records clean and not incorporate transaction 
verification. That's why the transaction verification has the pre-verify (add 
to the txn manager) and post verification (tidy the results and put them in a 
form for the post verification callback).
   
   If we want to move this code out of KafkaApis, I think it only makes sense 
to make a separate method. The other stages need to remain generic for the 
group coordinator usage.



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

Reply via email to