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
