hachikuji commented on a change in pull request #9912:
URL: https://github.com/apache/kafka/pull/9912#discussion_r561189636
##########
File path: core/src/test/scala/unit/kafka/server/KafkaApisTest.scala
##########
@@ -227,37 +225,33 @@ class KafkaApisTest {
authorizeResource(authorizer, operation, ResourceType.TOPIC, resourceName,
AuthorizationResult.ALLOWED)
- val capturedResponse = expectNoThrottling()
-
val configResource = new ConfigResource(ConfigResource.Type.TOPIC,
resourceName)
EasyMock.expect(adminManager.alterConfigs(anyObject(), EasyMock.eq(false)))
.andAnswer(() => {
Map(configResource -> alterConfigHandler.apply())
})
- EasyMock.replay(replicaManager, clientRequestQuotaManager, requestChannel,
authorizer,
- adminManager, controller)
-
val configs = Map(
configResource -> new AlterConfigsRequest.Config(
Seq(new AlterConfigsRequest.ConfigEntry("foo", "bar")).asJava))
val alterConfigsRequest = new AlterConfigsRequest.Builder(configs.asJava,
false).build(requestHeader.apiVersion)
val request = buildRequestWithEnvelope(alterConfigsRequest,
fromPrivilegedListener = true)
+ val capturedResponse = EasyMock.newCapture[AbstractResponse]()
+ val capturedRequest = EasyMock.newCapture[RequestChannel.Request]()
- createKafkaApis(authorizer = Some(authorizer), enableForwarding =
true).handle(request)
-
- val envelopeRequest = request.body[EnvelopeRequest]
- val response = readResponse(envelopeRequest, capturedResponse)
- .asInstanceOf[EnvelopeResponse]
-
- assertEquals(Errors.NONE, response.error)
Review comment:
There remains some awkwardness in the handling of envelope requests.
Basically the flow is like this:
1. KafkaApis.handle(envelope(alterConfigRequest))
2. KafkaApis.handle(alterConfigRequest)
3. RequestChannel.sendResponse(alterConfigResponse)
4. Request.buildResponseSend() -> envelope(alterConfigResponse)
Previously `KafkaApisTest` had to work by parsing the response send, so we
had to unwrap the envelope. But now we get the direct call to `sendResponse`
with the embedded `AbstractResponse` instance. So basically we do not hit step
4 anymore.
I think a nicer way to structure this which we can consider separately is to
change `KafkaApis.handle` so that it takes a callback rather than assuming that
responses are sent directly to the request channel. Then when we make the
recursive call to `handle` after unwrapping the envelope, we can provide a
callback which wraps the underlying response with the response envelope.
Alternatively, we can have `KafkaApis.handle` return a `CompletableFuture`. The
main point is that we allow for some custom behavior when the response is ready.
----------------------------------------------------------------
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:
us...@infra.apache.org
