TaiJuWu commented on code in PR #18454:
URL: https://github.com/apache/kafka/pull/18454#discussion_r1908160414
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -2619,16 +2619,6 @@ class KafkaApis(val requestChannel: RequestChannel,
aclApis.handleDescribeAcls(request)
}
- def handleCreateAcls(request: RequestChannel.Request): Unit = {
- metadataSupport.requireZkOrThrow(KafkaApis.shouldAlwaysForward(request))
- aclApis.handleCreateAcls(request)
- }
-
- def handleDeleteAcls(request: RequestChannel.Request): Unit = {
- metadataSupport.requireZkOrThrow(KafkaApis.shouldAlwaysForward(request))
- aclApis.handleDeleteAcls(request)
- }
-
Review Comment:
We can just throw exception within the two method.
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -223,8 +223,8 @@ class KafkaApis(val requestChannel: RequestChannel,
case ApiKeys.WRITE_TXN_MARKERS =>
handleWriteTxnMarkersRequest(request, requestLocal)
case ApiKeys.TXN_OFFSET_COMMIT =>
handleTxnOffsetCommitRequest(request, requestLocal).exceptionally(handleError)
case ApiKeys.DESCRIBE_ACLS => handleDescribeAcls(request)
- case ApiKeys.CREATE_ACLS => maybeForwardToController(request,
handleCreateAcls)
- case ApiKeys.DELETE_ACLS => maybeForwardToController(request,
handleDeleteAcls)
+ case ApiKeys.CREATE_ACLS => maybeForwardToController(request,
forwardToControllerOrFail)
+ case ApiKeys.DELETE_ACLS => maybeForwardToController(request,
forwardToControllerOrFail)
Review Comment:
Please revert these change.
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -10859,19 +10847,6 @@ class KafkaApisTest extends Logging {
verifyShouldAlwaysForwardErrorMessage(kafkaApis.handleDeleteTopicsRequest)
}
- @Test
- def testRaftShouldAlwaysForwardCreateAcls(): Unit = {
- metadataCache = MetadataCache.kRaftMetadataCache(brokerId, () =>
KRaftVersion.KRAFT_VERSION_0)
- kafkaApis = createKafkaApis(raftSupport = true)
- verifyShouldAlwaysForwardErrorMessage(kafkaApis.handleCreateAcls)
- }
-
- @Test
- def testRaftShouldAlwaysForwardDeleteAcls(): Unit = {
- metadataCache = MetadataCache.kRaftMetadataCache(brokerId, () =>
KRaftVersion.KRAFT_VERSION_0)
- kafkaApis = createKafkaApis(raftSupport = true)
- verifyShouldAlwaysForwardErrorMessage(kafkaApis.handleDeleteAcls)
- }
Review Comment:
ditto
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -1196,18 +1196,6 @@ class KafkaApisTest extends Logging {
assertEquals(expectedTopicConfigErrorCodes, actualTopicConfigErrorCodes)
}
- @Test
- def testCreateAclWithForwarding(): Unit = {
- val requestBuilder = new CreateAclsRequest.Builder(new
CreateAclsRequestData())
- testForwardableApi(ApiKeys.CREATE_ACLS, requestBuilder)
- }
-
- @Test
- def testDeleteAclWithForwarding(): Unit = {
- val requestBuilder = new DeleteAclsRequest.Builder(new
DeleteAclsRequestData())
- testForwardableApi(ApiKeys.DELETE_ACLS, requestBuilder)
- }
-
Review Comment:
Please revert this change. These tests should be retained.
--
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
