dajac commented on a change in pull request #9526:
URL: https://github.com/apache/kafka/pull/9526#discussion_r542308308
##########
File path: core/src/main/scala/kafka/network/RequestChannel.scala
##########
@@ -123,9 +131,9 @@ object RequestChannel extends Logging {
}
}
- def responseString(response: AbstractResponse): Option[String] = {
+ def responseNode(response: AbstractResponse): Option[JsonNode] = {
if (RequestChannel.isRequestLoggingEnabled)
- Some(response.toString)
+ Some(RequestConvertToJson.response(response, context.apiVersion()))
Review comment:
nit: `context.apiVersion()` -> `context.apiVersion`
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -97,7 +98,8 @@ import scala.annotation.nowarn
/**
* Logic to handle the various Kafka requests
*/
-class KafkaApis(val requestChannel: RequestChannel,
+class
+KafkaApis(val requestChannel: RequestChannel,
Review comment:
This is likely a mistake.
##########
File path: core/src/main/scala/kafka/tools/TestRaftRequestHandler.scala
##########
@@ -39,7 +40,8 @@ class TestRaftRequestHandler(
override def handle(request: RequestChannel.Request): Unit = {
try {
- trace(s"Handling request:${request.requestDesc(true)} from connection
${request.context.connectionId};" +
+ trace(s"Handling
request:${RequestConvertToJson.requestDesc(request.header, request.requestLog,
request.isForwarded)} " +
Review comment:
We should also revert this one.
##########
File path: core/src/main/scala/kafka/network/RequestConvertToJson.scala
##########
@@ -0,0 +1,216 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package kafka.network
+
+import com.fasterxml.jackson.databind.JsonNode
+import com.fasterxml.jackson.databind.node.{BooleanNode, DoubleNode,
JsonNodeFactory, LongNode, ObjectNode, TextNode}
+import kafka.network.RequestChannel.Session
+import org.apache.kafka.common.message._
+import org.apache.kafka.common.network.ClientInformation
+import org.apache.kafka.common.requests._
+
+object RequestConvertToJson {
+ def request(request: AbstractRequest): JsonNode = {
+ request match {
+ case req: AddOffsetsToTxnRequest =>
AddOffsetsToTxnRequestDataJsonConverter.write(req.data, request.version)
+ case req: AddPartitionsToTxnRequest =>
AddPartitionsToTxnRequestDataJsonConverter.write(req.data, request.version)
+ case req: AlterClientQuotasRequest =>
AlterClientQuotasRequestDataJsonConverter.write(req.data, request.version)
+ case req: AlterConfigsRequest =>
AlterConfigsRequestDataJsonConverter.write(req.data, request.version)
+ case req: AlterIsrRequest =>
AlterIsrRequestDataJsonConverter.write(req.data, request.version)
+ case req: AlterPartitionReassignmentsRequest =>
AlterPartitionReassignmentsRequestDataJsonConverter.write(req.data,
request.version)
+ case req: AlterReplicaLogDirsRequest =>
AlterReplicaLogDirsRequestDataJsonConverter.write(req.data, request.version)
+ case res: AlterUserScramCredentialsRequest =>
AlterUserScramCredentialsRequestDataJsonConverter.write(res.data,
request.version)
+ case req: ApiVersionsRequest =>
ApiVersionsRequestDataJsonConverter.write(req.data, request.version)
+ case req: BeginQuorumEpochRequest =>
BeginQuorumEpochRequestDataJsonConverter.write(req.data, request.version)
+ case req: ControlledShutdownRequest =>
ControlledShutdownRequestDataJsonConverter.write(req.data, request.version)
+ case req: CreateAclsRequest =>
CreateAclsRequestDataJsonConverter.write(req.data, request.version)
+ case req: CreateDelegationTokenRequest =>
CreateDelegationTokenRequestDataJsonConverter.write(req.data, request.version)
+ case req: CreatePartitionsRequest =>
CreatePartitionsRequestDataJsonConverter.write(req.data, request.version)
+ case req: CreateTopicsRequest =>
CreateTopicsRequestDataJsonConverter.write(req.data, request.version)
+ case req: DeleteAclsRequest =>
DeleteAclsRequestDataJsonConverter.write(req.data, request.version)
+ case req: DeleteGroupsRequest =>
DeleteGroupsRequestDataJsonConverter.write(req.data, request.version)
+ case req: DeleteRecordsRequest =>
DeleteRecordsRequestDataJsonConverter.write(req.data, request.version)
+ case req: DeleteTopicsRequest =>
DeleteTopicsRequestDataJsonConverter.write(req.data, request.version)
+ case req: DescribeAclsRequest =>
DescribeAclsRequestDataJsonConverter.write(req.data, request.version)
+ case req: DescribeClientQuotasRequest =>
DescribeClientQuotasRequestDataJsonConverter.write(req.data, request.version)
+ case req: DescribeConfigsRequest =>
DescribeConfigsRequestDataJsonConverter.write(req.data, request.version)
+ case req: DescribeDelegationTokenRequest =>
DescribeDelegationTokenRequestDataJsonConverter.write(req.data, request.version)
+ case req: DescribeGroupsRequest =>
DescribeGroupsRequestDataJsonConverter.write(req.data, request.version)
+ case req: DescribeLogDirsRequest =>
DescribeLogDirsRequestDataJsonConverter.write(req.data, request.version)
+ case req: DescribeQuorumRequest =>
DescribeQuorumRequestDataJsonConverter.write(req.data, request.version)
+ case res: DescribeUserScramCredentialsRequest =>
DescribeUserScramCredentialsRequestDataJsonConverter.write(res.data,
request.version)
+ case req: ElectLeadersRequest =>
ElectLeadersRequestDataJsonConverter.write(req.data, request.version)
+ case req: EndTxnRequest =>
EndTxnRequestDataJsonConverter.write(req.data, request.version)
+ case req: EndQuorumEpochRequest =>
EndQuorumEpochRequestDataJsonConverter.write(req.data, request.version)
+ case req: EnvelopeRequest =>
EnvelopeRequestDataJsonConverter.write(req.data, request.version)
+ case req: ExpireDelegationTokenRequest =>
ExpireDelegationTokenRequestDataJsonConverter.write(req.data, request.version)
+ case req: FetchRequest => FetchRequestDataJsonConverter.write(req.data,
request.version)
+ case req: FindCoordinatorRequest =>
FindCoordinatorRequestDataJsonConverter.write(req.data, request.version)
+ case req: HeartbeatRequest =>
HeartbeatRequestDataJsonConverter.write(req.data, request.version)
+ case req: IncrementalAlterConfigsRequest =>
IncrementalAlterConfigsRequestDataJsonConverter.write(req.data, request.version)
+ case req: InitProducerIdRequest =>
InitProducerIdRequestDataJsonConverter.write(req.data, request.version)
+ case req: JoinGroupRequest =>
JoinGroupRequestDataJsonConverter.write(req.data, request.version)
+ case req: LeaderAndIsrRequest =>
LeaderAndIsrRequestDataJsonConverter.write(req.data, request.version)
+ case req: LeaveGroupRequest =>
LeaveGroupRequestDataJsonConverter.write(req.data, request.version)
+ case req: ListGroupsRequest =>
ListGroupsRequestDataJsonConverter.write(req.data, request.version)
+ case req: ListOffsetRequest =>
ListOffsetRequestDataJsonConverter.write(req.data, request.version)
+ case req: ListPartitionReassignmentsRequest =>
ListPartitionReassignmentsRequestDataJsonConverter.write(req.data,
request.version)
+ case req: MetadataRequest =>
MetadataRequestDataJsonConverter.write(req.data, request.version)
+ case req: OffsetCommitRequest =>
OffsetCommitRequestDataJsonConverter.write(req.data, request.version)
+ case req: OffsetDeleteRequest =>
OffsetDeleteRequestDataJsonConverter.write(req.data, request.version)
+ case req: OffsetFetchRequest =>
OffsetFetchRequestDataJsonConverter.write(req.data, request.version)
+ case req: OffsetsForLeaderEpochRequest =>
OffsetForLeaderEpochRequestDataJsonConverter.write(req.data, request.version)
+ case req: ProduceRequest =>
ProduceRequestDataJsonConverter.write(req.data, request.version, false)
+ case req: RenewDelegationTokenRequest =>
RenewDelegationTokenRequestDataJsonConverter.write(req.data, request.version)
+ case req: SaslAuthenticateRequest =>
SaslAuthenticateRequestDataJsonConverter.write(req.data, request.version)
+ case req: SaslHandshakeRequest =>
SaslHandshakeRequestDataJsonConverter.write(req.data, request.version)
+ case req: StopReplicaRequest =>
StopReplicaRequestDataJsonConverter.write(req.data, request.version)
+ case req: SyncGroupRequest =>
SyncGroupRequestDataJsonConverter.write(req.data, request.version)
+ case req: TxnOffsetCommitRequest =>
TxnOffsetCommitRequestDataJsonConverter.write(req.data, request.version)
+ case req: UpdateFeaturesRequest =>
UpdateFeaturesRequestDataJsonConverter.write(req.data, request.version)
+ case req: UpdateMetadataRequest =>
UpdateMetadataRequestDataJsonConverter.write(req.data, request.version)
+ case req: VoteRequest => VoteRequestDataJsonConverter.write(req.data,
request.version)
+ case req: WriteTxnMarkersRequest =>
WriteTxnMarkersRequestDataJsonConverter.write(req.data, request.version)
+ case _ => throw new IllegalStateException(s"ApiKey ${request.apiKey} is
not currently handled in `request`, the " +
+ "code should be updated to do so.");
+ }
+ }
+
+ def response(response: AbstractResponse, version: Short): JsonNode = {
+ response match {
+ case res: AddOffsetsToTxnResponse =>
AddOffsetsToTxnResponseDataJsonConverter.write(res.data, version)
+ case res: AddPartitionsToTxnResponse =>
AddPartitionsToTxnResponseDataJsonConverter.write(res.data, version)
+ case res: AlterClientQuotasResponse =>
AlterClientQuotasResponseDataJsonConverter.write(res.data, version)
+ case res: AlterConfigsResponse =>
AlterConfigsResponseDataJsonConverter.write(res.data, version)
+ case res: AlterIsrResponse =>
AlterIsrResponseDataJsonConverter.write(res.data, version)
+ case res: AlterPartitionReassignmentsResponse =>
AlterPartitionReassignmentsResponseDataJsonConverter.write(res.data, version)
+ case res: AlterReplicaLogDirsResponse =>
AlterReplicaLogDirsResponseDataJsonConverter.write(res.data, version)
+ case res: AlterUserScramCredentialsResponse =>
AlterUserScramCredentialsResponseDataJsonConverter.write(res.data, version)
+ case res: ApiVersionsResponse =>
ApiVersionsResponseDataJsonConverter.write(res.data, version)
+ case res: BeginQuorumEpochResponse =>
BeginQuorumEpochResponseDataJsonConverter.write(res.data, version)
+ case res: ControlledShutdownResponse =>
ControlledShutdownResponseDataJsonConverter.write(res.data, version)
+ case res: CreateAclsResponse =>
CreateAclsResponseDataJsonConverter.write(res.data, version)
+ case res: CreateDelegationTokenResponse =>
CreateDelegationTokenResponseDataJsonConverter.write(res.data, version)
+ case res: CreatePartitionsResponse =>
CreatePartitionsResponseDataJsonConverter.write(res.data, version)
+ case res: CreateTopicsResponse =>
CreateTopicsResponseDataJsonConverter.write(res.data, version)
+ case res: DeleteAclsResponse =>
DeleteAclsResponseDataJsonConverter.write(res.data, version)
+ case res: DeleteGroupsResponse =>
DeleteGroupsResponseDataJsonConverter.write(res.data, version)
+ case res: DeleteRecordsResponse =>
DeleteRecordsResponseDataJsonConverter.write(res.data, version)
+ case res: DeleteTopicsResponse =>
DeleteTopicsResponseDataJsonConverter.write(res.data, version)
+ case res: DescribeAclsResponse =>
DescribeAclsResponseDataJsonConverter.write(res.data, version)
+ case res: DescribeClientQuotasResponse =>
DescribeClientQuotasResponseDataJsonConverter.write(res.data, version)
+ case res: DescribeConfigsResponse =>
DescribeConfigsResponseDataJsonConverter.write(res.data, version)
+ case res: DescribeDelegationTokenResponse =>
DescribeDelegationTokenResponseDataJsonConverter.write(res.data, version)
+ case res: DescribeGroupsResponse =>
DescribeGroupsResponseDataJsonConverter.write(res.data, version)
+ case res: DescribeLogDirsResponse =>
DescribeLogDirsResponseDataJsonConverter.write(res.data, version)
+ case res: DescribeQuorumResponse =>
DescribeQuorumResponseDataJsonConverter.write(res.data, version)
+ case res: DescribeUserScramCredentialsResponse =>
DescribeUserScramCredentialsResponseDataJsonConverter.write(res.data, version)
+ case res: ElectLeadersResponse =>
ElectLeadersResponseDataJsonConverter.write(res.data, version)
+ case res: EndTxnResponse =>
EndTxnResponseDataJsonConverter.write(res.data, version)
+ case res: EndQuorumEpochResponse =>
EndQuorumEpochResponseDataJsonConverter.write(res.data, version)
+ case res: EnvelopeResponse =>
EnvelopeResponseDataJsonConverter.write(res.data, version)
+ case res: ExpireDelegationTokenResponse =>
ExpireDelegationTokenResponseDataJsonConverter.write(res.data, version)
+ case res: FetchResponse[_] =>
FetchResponseDataJsonConverter.write(res.data, version, false)
+ case res: FindCoordinatorResponse =>
FindCoordinatorResponseDataJsonConverter.write(res.data, version)
+ case res: HeartbeatResponse =>
HeartbeatResponseDataJsonConverter.write(res.data, version)
+ case res: IncrementalAlterConfigsResponse =>
IncrementalAlterConfigsResponseDataJsonConverter.write(res.data, version)
+ case res: InitProducerIdResponse =>
InitProducerIdResponseDataJsonConverter.write(res.data, version)
+ case res: JoinGroupResponse =>
JoinGroupResponseDataJsonConverter.write(res.data, version)
+ case res: LeaderAndIsrResponse =>
LeaderAndIsrResponseDataJsonConverter.write(res.data, version)
+ case res: LeaveGroupResponse =>
LeaveGroupResponseDataJsonConverter.write(res.data, version)
+ case res: ListGroupsResponse =>
ListGroupsResponseDataJsonConverter.write(res.data, version)
+ case res: ListOffsetResponse =>
ListOffsetResponseDataJsonConverter.write(res.data, version)
+ case res: ListPartitionReassignmentsResponse =>
ListPartitionReassignmentsResponseDataJsonConverter.write(res.data, version)
+ case res: MetadataResponse =>
MetadataResponseDataJsonConverter.write(res.data, version)
+ case res: OffsetCommitResponse =>
OffsetCommitResponseDataJsonConverter.write(res.data, version)
+ case res: OffsetDeleteResponse =>
OffsetDeleteResponseDataJsonConverter.write(res.data, version)
+ case res: OffsetFetchResponse =>
OffsetFetchResponseDataJsonConverter.write(res.data, version)
+ case res: OffsetsForLeaderEpochResponse =>
OffsetForLeaderEpochResponseDataJsonConverter.write(res.data, version)
+ case res: ProduceResponse =>
ProduceResponseDataJsonConverter.write(res.data, version)
+ case res: RenewDelegationTokenResponse =>
RenewDelegationTokenResponseDataJsonConverter.write(res.data, version)
+ case res: SaslAuthenticateResponse =>
SaslAuthenticateResponseDataJsonConverter.write(res.data, version)
+ case res: SaslHandshakeResponse =>
SaslHandshakeResponseDataJsonConverter.write(res.data, version)
+ case res: StopReplicaResponse =>
StopReplicaResponseDataJsonConverter.write(res.data, version)
+ case res: SyncGroupResponse =>
SyncGroupResponseDataJsonConverter.write(res.data, version)
+ case res: TxnOffsetCommitResponse =>
TxnOffsetCommitResponseDataJsonConverter.write(res.data, version)
+ case res: UpdateFeaturesResponse =>
UpdateFeaturesResponseDataJsonConverter.write(res.data, version)
+ case res: UpdateMetadataResponse =>
UpdateMetadataResponseDataJsonConverter.write(res.data, version)
+ case res: WriteTxnMarkersResponse =>
WriteTxnMarkersResponseDataJsonConverter.write(res.data, version)
+ case res: VoteResponse => VoteResponseDataJsonConverter.write(res.data,
version)
+ case _ => throw new IllegalStateException(s"ApiKey ${response.apiKey} is
not currently handled in `response`, the " +
+ "code should be updated to do so.");
+ }
+ }
+
+ def requestHeaderNode(header: RequestHeader): JsonNode = {
+ val node = RequestHeaderDataJsonConverter.write(header.data,
header.headerVersion, false).asInstanceOf[ObjectNode]
+ node.set("requestApiKeyName", new TextNode(header.apiKey.toString))
+ node
+ }
+
+ def requestContextNode(context: RequestContext): JsonNode = {
Review comment:
It seems that this one is not used anymore.
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -183,7 +185,8 @@ class KafkaApis(val requestChannel: RequestChannel,
*/
override def handle(request: RequestChannel.Request): Unit = {
try {
- trace(s"Handling request:${request.requestDesc(true)} from connection
${request.context.connectionId};" +
+ trace(s"Handling
request:${RequestConvertToJson.requestDesc(request.header, request.requestLog,
request.isForwarded)} " +
Review comment:
We should also revert this one.
##########
File path: core/src/main/scala/kafka/network/RequestChannel.scala
##########
@@ -200,7 +208,7 @@ object RequestChannel extends Logging {
}
}
- trace(s"Processor $processor received request: ${requestDesc(true)}")
+ trace(s"Processor $processor received request:
${RequestConvertToJson.requestDesc(header, requestLog, isForwarded)}")
Review comment:
As discussed offline, I think that we should revert this one. We want to
use JSON in the request log (aka `requestLogger`), not as a replacement of all
the `toString` usages.
##########
File path: core/src/test/scala/unit/kafka/network/RequestConvertToJsonTest.scala
##########
@@ -0,0 +1,154 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package kafka.network
+
+import java.net.InetAddress
+import java.nio.ByteBuffer
+
+import com.fasterxml.jackson.databind.node.{DoubleNode, LongNode, ObjectNode,
TextNode}
+import kafka.network
+import org.apache.kafka.common.memory.MemoryPool
+import org.apache.kafka.common.message._
+import org.apache.kafka.common.network.{ClientInformation, ListenerName,
NetworkSend}
+import org.junit.Test
+import org.apache.kafka.common.protocol.{ApiKeys, ByteBufferAccessor,
ObjectSerializationCache}
+import org.apache.kafka.common.requests._
+import org.apache.kafka.common.security.auth.{KafkaPrincipal, SecurityProtocol}
+import org.easymock.EasyMock.createNiceMock
+import org.junit.Assert.assertEquals
+
+import scala.collection.mutable.ArrayBuffer
+
+class RequestConvertToJsonTest {
+
+ @Test
+ def testAllRequestTypesHandled(): Unit = {
+ val unhandledKeys = ArrayBuffer[String]()
+ ApiKeys.values().foreach { key => {
+ val version: Short = key.latestVersion()
+ val cache = new ObjectSerializationCache
+ val message = key match {
+ case ApiKeys.DESCRIBE_ACLS =>
+
ApiMessageType.fromApiKey(key.id).newRequest().asInstanceOf[DescribeAclsRequestData]
+
.setPatternTypeFilter(1).setResourceTypeFilter(1).setPermissionType(1).setOperation(1)
+ case _ =>
+ ApiMessageType.fromApiKey(key.id).newRequest()
+ }
+ val messageSize = message.size(cache, version)
+ val bytes = new ByteBufferAccessor(ByteBuffer.allocate(messageSize))
+ message.write(bytes, cache, version)
+ bytes.flip()
+
+ val req = AbstractRequest.parseRequest(key, version,
bytes.buffer).request
+ try {
+ RequestConvertToJson.request(req)
+ } catch {
+ case _ : IllegalStateException => unhandledKeys += key.toString
+ }
+ }}
+ assertEquals("Unhandled request keys", ArrayBuffer.empty, unhandledKeys)
+ }
+
Review comment:
Could we also add unit tests for `RequestConvertToJson.requestDesc`,
`RequestConvertToJson.requestHeaderNode` and
`RequestConvertToJson.clientInfoNode`?
##########
File path:
generator/src/main/java/org/apache/kafka/message/JsonConverterGenerator.java
##########
@@ -377,11 +387,24 @@ private void generateVariableLengthTargetToJson(Target
target, Versions versions
headerGenerator.addImport(MessageGenerator.ARRAYS_CLASS);
buffer.printf("%s;%n", target.assignmentStatement(
String.format("new BinaryNode(Arrays.copyOf(%s,
%s.length))",
- target.sourceVariable(), target.sourceVariable())));
+ target.sourceVariable(),
target.sourceVariable())));
}
} else if (target.field().type().isRecords()) {
headerGenerator.addImport(MessageGenerator.BINARY_NODE_CLASS);
+ headerGenerator.addImport(MessageGenerator.INT_NODE_CLASS);
+ // KIP-673: When logging requests/responses, we do not serialize
the record, instead we
+ // output its sizeInBytes, because outputting the bytes is not
very useful and can be
+ // quite expensive. Otherwise, we will serialize the record.
+ buffer.printf("if (_serializeRecords) {%n");
+ buffer.incrementIndent();
buffer.printf("%s;%n", target.assignmentStatement("new
BinaryNode(new byte[]{})"));
+ buffer.decrementIndent();
+ buffer.printf("} else {%n");
+ buffer.incrementIndent();
+ buffer.printf("%s;%n", target.assignmentStatement(
Review comment:
Would something like the following work?
```
buffer.printf("_node.set(\"%sSizeInBytes\", new
IntNode(%s.sizeInBytes()));%n",
target.field().camelCaseName(),
target.sourceVariable());
```
----------------------------------------------------------------
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
