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, but wanted to minimize methods with a ton of arguments that do similar things. The naming starts to get confusing as well. An alternative I can think of is to create another method in ReplicaManager for the transaction produce path, but it will have all the append arguments and just call this same callback under the hood. 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