-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-10818: Skip conversion to Struct when serializing generated requests/responses
#7409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7c6ada5
bc333a7
10b01b9
7f66cda
8be8cef
a84d3d8
17af66b
9a93b0b
859041a
98dbf1d
daa6dbb
a36bbb3
a1bf993
e00cace
56cba5f
5e0f689
d6cb097
9317347
1b07c53
f57d04d
186aad4
ebf63f7
4e4f665
1869826
00204bc
c430055
5d24335
d834f14
fb876a1
2e36e92
18d9096
0b8ea15
aa1735f
fe9eccb
5b7695b
d5cbad3
02a98d8
5023f3e
6a20dab
ff108fa
bf7f76d
0ad34fb
dee6874
b17d1bf
830d46c
8e719a4
de995be
ce9bd0b
d3c8fa6
b7490af
55a6110
7ad198f
cfbbede
f05f217
4829f7c
420a74d
cfd0c66
de6dca1
ad0e0d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,14 +17,12 @@ | |
| package org.apache.kafka.common.requests; | ||
|
|
||
| import org.apache.kafka.common.errors.UnsupportedVersionException; | ||
| import org.apache.kafka.common.message.FetchRequestData; | ||
| import org.apache.kafka.common.message.AlterIsrRequestData; | ||
| import org.apache.kafka.common.message.ProduceRequestData; | ||
| import org.apache.kafka.common.network.NetworkSend; | ||
| import org.apache.kafka.common.network.Send; | ||
| import org.apache.kafka.common.protocol.ApiKeys; | ||
| import org.apache.kafka.common.protocol.Errors; | ||
| import org.apache.kafka.common.protocol.types.Struct; | ||
| import org.apache.kafka.common.protocol.Message; | ||
| import org.apache.kafka.common.protocol.ObjectSerializationCache; | ||
| import org.apache.kafka.common.protocol.SendBuilder; | ||
|
|
||
| import java.nio.ByteBuffer; | ||
| import java.util.Map; | ||
|
|
@@ -79,13 +77,13 @@ public T build() { | |
| } | ||
|
|
||
| private final short version; | ||
| public final ApiKeys api; | ||
| private final ApiKeys apiKey; | ||
|
|
||
| public AbstractRequest(ApiKeys api, short version) { | ||
| if (!api.isVersionSupported(version)) | ||
| throw new UnsupportedVersionException("The " + api + " protocol does not support version " + version); | ||
| public AbstractRequest(ApiKeys apiKey, short version) { | ||
| if (!apiKey.isVersionSupported(version)) | ||
| throw new UnsupportedVersionException("The " + apiKey + " protocol does not support version " + version); | ||
| this.version = version; | ||
| this.api = api; | ||
| this.apiKey = apiKey; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -95,21 +93,33 @@ public short version() { | |
| return version; | ||
| } | ||
|
|
||
| public Send toSend(String destination, RequestHeader header) { | ||
| return new NetworkSend(destination, serialize(header)); | ||
| public ApiKeys apiKey() { | ||
| return apiKey; | ||
| } | ||
|
|
||
| /** | ||
| * Use with care, typically {@link #toSend(String, RequestHeader)} should be used instead. | ||
| */ | ||
| public ByteBuffer serialize(RequestHeader header) { | ||
| return RequestUtils.serialize(header.toStruct(), toStruct()); | ||
| public final Send toSend(String destination, RequestHeader header) { | ||
| return SendBuilder.buildRequestSend(destination, header, data()); | ||
| } | ||
|
|
||
| // Visible for testing | ||
| public final ByteBuffer serializeWithHeader(RequestHeader header) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about moving this method to test code?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto for following "Visible for testing" code
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW, this method is duplicate to TestUtils#serializeRequestHeader
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method serializes the request with the header while the test method serializes the header only. So, not a duplicate, right? Personally, I think it's useful for the class to be symmetric: it can parse from a buffer and serialize to a buffer. Do you have a concern that it may be misused?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a reasonable argument that
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yes. Also the various serialization methods are a bit chaos to me :(
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should move all testing-only serialization to a single utils (in test scope). I grep "static ByteBuffer" + "request/response" and
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a refactoring that consolidates the logic around
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have these changes locally, so I can submit them immediately after. |
||
| return RequestUtils.serialize(header.data(), header.headerVersion(), data(), version); | ||
| } | ||
|
|
||
| protected abstract Message data(); | ||
|
|
||
| // Visible for testing | ||
| public final ByteBuffer serializeBody() { | ||
| return RequestUtils.serialize(null, (short) 0, data(), version); | ||
| } | ||
|
|
||
| protected abstract Struct toStruct(); | ||
| // Visible for testing | ||
| final int sizeInBytes() { | ||
| return data().size(new ObjectSerializationCache(), version); | ||
| } | ||
|
|
||
| public String toString(boolean verbose) { | ||
| return toStruct().toString(); | ||
| return data().toString(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this toString variety still useful?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so. Do you think it's not?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, what do you mean by
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems only
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The verbosity flag is used for request logging. Produce and fetch are the most important ones. The rest can be ignored. I don't see the benefit in removing this functionality. |
||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -144,126 +154,131 @@ public Map<Errors, Integer> errorCounts(Throwable e) { | |
| /** | ||
| * Factory method for getting a request object based on ApiKey ID and a version | ||
| */ | ||
| public static AbstractRequest parseRequest(ApiKeys apiKey, short apiVersion, Struct struct) { | ||
| public static RequestAndSize parseRequest(ApiKeys apiKey, short apiVersion, ByteBuffer buffer) { | ||
| int bufferSize = buffer.remaining(); | ||
| return new RequestAndSize(doParseRequest(apiKey, apiVersion, buffer), bufferSize); | ||
| } | ||
|
|
||
| private static AbstractRequest doParseRequest(ApiKeys apiKey, short apiVersion, ByteBuffer buffer) { | ||
| switch (apiKey) { | ||
| case PRODUCE: | ||
| return new ProduceRequest(new ProduceRequestData(struct, apiVersion), apiVersion); | ||
| return ProduceRequest.parse(buffer, apiVersion); | ||
| case FETCH: | ||
| return new FetchRequest(new FetchRequestData(struct, apiVersion), apiVersion); | ||
| return FetchRequest.parse(buffer, apiVersion); | ||
| case LIST_OFFSETS: | ||
| return new ListOffsetRequest(struct, apiVersion); | ||
| return ListOffsetRequest.parse(buffer, apiVersion); | ||
| case METADATA: | ||
| return new MetadataRequest(struct, apiVersion); | ||
| return MetadataRequest.parse(buffer, apiVersion); | ||
| case OFFSET_COMMIT: | ||
| return new OffsetCommitRequest(struct, apiVersion); | ||
| return OffsetCommitRequest.parse(buffer, apiVersion); | ||
| case OFFSET_FETCH: | ||
| return new OffsetFetchRequest(struct, apiVersion); | ||
| return OffsetFetchRequest.parse(buffer, apiVersion); | ||
| case FIND_COORDINATOR: | ||
| return new FindCoordinatorRequest(struct, apiVersion); | ||
| return FindCoordinatorRequest.parse(buffer, apiVersion); | ||
| case JOIN_GROUP: | ||
| return new JoinGroupRequest(struct, apiVersion); | ||
| return JoinGroupRequest.parse(buffer, apiVersion); | ||
| case HEARTBEAT: | ||
| return new HeartbeatRequest(struct, apiVersion); | ||
| return HeartbeatRequest.parse(buffer, apiVersion); | ||
| case LEAVE_GROUP: | ||
| return new LeaveGroupRequest(struct, apiVersion); | ||
| return LeaveGroupRequest.parse(buffer, apiVersion); | ||
| case SYNC_GROUP: | ||
| return new SyncGroupRequest(struct, apiVersion); | ||
| return SyncGroupRequest.parse(buffer, apiVersion); | ||
| case STOP_REPLICA: | ||
| return new StopReplicaRequest(struct, apiVersion); | ||
| return StopReplicaRequest.parse(buffer, apiVersion); | ||
| case CONTROLLED_SHUTDOWN: | ||
| return new ControlledShutdownRequest(struct, apiVersion); | ||
| return ControlledShutdownRequest.parse(buffer, apiVersion); | ||
| case UPDATE_METADATA: | ||
| return new UpdateMetadataRequest(struct, apiVersion); | ||
| return UpdateMetadataRequest.parse(buffer, apiVersion); | ||
| case LEADER_AND_ISR: | ||
| return new LeaderAndIsrRequest(struct, apiVersion); | ||
| return LeaderAndIsrRequest.parse(buffer, apiVersion); | ||
| case DESCRIBE_GROUPS: | ||
| return new DescribeGroupsRequest(struct, apiVersion); | ||
| return DescribeGroupsRequest.parse(buffer, apiVersion); | ||
| case LIST_GROUPS: | ||
| return new ListGroupsRequest(struct, apiVersion); | ||
| return ListGroupsRequest.parse(buffer, apiVersion); | ||
| case SASL_HANDSHAKE: | ||
| return new SaslHandshakeRequest(struct, apiVersion); | ||
| return SaslHandshakeRequest.parse(buffer, apiVersion); | ||
| case API_VERSIONS: | ||
| return new ApiVersionsRequest(struct, apiVersion); | ||
| return ApiVersionsRequest.parse(buffer, apiVersion); | ||
| case CREATE_TOPICS: | ||
| return new CreateTopicsRequest(struct, apiVersion); | ||
| return CreateTopicsRequest.parse(buffer, apiVersion); | ||
| case DELETE_TOPICS: | ||
| return new DeleteTopicsRequest(struct, apiVersion); | ||
| return DeleteTopicsRequest.parse(buffer, apiVersion); | ||
| case DELETE_RECORDS: | ||
| return new DeleteRecordsRequest(struct, apiVersion); | ||
| return DeleteRecordsRequest.parse(buffer, apiVersion); | ||
| case INIT_PRODUCER_ID: | ||
| return new InitProducerIdRequest(struct, apiVersion); | ||
| return InitProducerIdRequest.parse(buffer, apiVersion); | ||
| case OFFSET_FOR_LEADER_EPOCH: | ||
| return new OffsetsForLeaderEpochRequest(struct, apiVersion); | ||
| return OffsetsForLeaderEpochRequest.parse(buffer, apiVersion); | ||
| case ADD_PARTITIONS_TO_TXN: | ||
| return new AddPartitionsToTxnRequest(struct, apiVersion); | ||
| return AddPartitionsToTxnRequest.parse(buffer, apiVersion); | ||
| case ADD_OFFSETS_TO_TXN: | ||
| return new AddOffsetsToTxnRequest(struct, apiVersion); | ||
| return AddOffsetsToTxnRequest.parse(buffer, apiVersion); | ||
| case END_TXN: | ||
| return new EndTxnRequest(struct, apiVersion); | ||
| return EndTxnRequest.parse(buffer, apiVersion); | ||
| case WRITE_TXN_MARKERS: | ||
| return new WriteTxnMarkersRequest(struct, apiVersion); | ||
| return WriteTxnMarkersRequest.parse(buffer, apiVersion); | ||
| case TXN_OFFSET_COMMIT: | ||
| return new TxnOffsetCommitRequest(struct, apiVersion); | ||
| return TxnOffsetCommitRequest.parse(buffer, apiVersion); | ||
| case DESCRIBE_ACLS: | ||
| return new DescribeAclsRequest(struct, apiVersion); | ||
| return DescribeAclsRequest.parse(buffer, apiVersion); | ||
| case CREATE_ACLS: | ||
| return new CreateAclsRequest(struct, apiVersion); | ||
| return CreateAclsRequest.parse(buffer, apiVersion); | ||
| case DELETE_ACLS: | ||
| return new DeleteAclsRequest(struct, apiVersion); | ||
| return DeleteAclsRequest.parse(buffer, apiVersion); | ||
| case DESCRIBE_CONFIGS: | ||
| return new DescribeConfigsRequest(struct, apiVersion); | ||
| return DescribeConfigsRequest.parse(buffer, apiVersion); | ||
| case ALTER_CONFIGS: | ||
| return new AlterConfigsRequest(struct, apiVersion); | ||
| return AlterConfigsRequest.parse(buffer, apiVersion); | ||
| case ALTER_REPLICA_LOG_DIRS: | ||
| return new AlterReplicaLogDirsRequest(struct, apiVersion); | ||
| return AlterReplicaLogDirsRequest.parse(buffer, apiVersion); | ||
| case DESCRIBE_LOG_DIRS: | ||
| return new DescribeLogDirsRequest(struct, apiVersion); | ||
| return DescribeLogDirsRequest.parse(buffer, apiVersion); | ||
| case SASL_AUTHENTICATE: | ||
| return new SaslAuthenticateRequest(struct, apiVersion); | ||
| return SaslAuthenticateRequest.parse(buffer, apiVersion); | ||
| case CREATE_PARTITIONS: | ||
| return new CreatePartitionsRequest(struct, apiVersion); | ||
| return CreatePartitionsRequest.parse(buffer, apiVersion); | ||
| case CREATE_DELEGATION_TOKEN: | ||
| return new CreateDelegationTokenRequest(struct, apiVersion); | ||
| return CreateDelegationTokenRequest.parse(buffer, apiVersion); | ||
| case RENEW_DELEGATION_TOKEN: | ||
| return new RenewDelegationTokenRequest(struct, apiVersion); | ||
| return RenewDelegationTokenRequest.parse(buffer, apiVersion); | ||
| case EXPIRE_DELEGATION_TOKEN: | ||
| return new ExpireDelegationTokenRequest(struct, apiVersion); | ||
| return ExpireDelegationTokenRequest.parse(buffer, apiVersion); | ||
| case DESCRIBE_DELEGATION_TOKEN: | ||
| return new DescribeDelegationTokenRequest(struct, apiVersion); | ||
| return DescribeDelegationTokenRequest.parse(buffer, apiVersion); | ||
| case DELETE_GROUPS: | ||
| return new DeleteGroupsRequest(struct, apiVersion); | ||
| return DeleteGroupsRequest.parse(buffer, apiVersion); | ||
| case ELECT_LEADERS: | ||
| return new ElectLeadersRequest(struct, apiVersion); | ||
| return ElectLeadersRequest.parse(buffer, apiVersion); | ||
| case INCREMENTAL_ALTER_CONFIGS: | ||
| return new IncrementalAlterConfigsRequest(struct, apiVersion); | ||
| return IncrementalAlterConfigsRequest.parse(buffer, apiVersion); | ||
| case ALTER_PARTITION_REASSIGNMENTS: | ||
| return new AlterPartitionReassignmentsRequest(struct, apiVersion); | ||
| return AlterPartitionReassignmentsRequest.parse(buffer, apiVersion); | ||
| case LIST_PARTITION_REASSIGNMENTS: | ||
| return new ListPartitionReassignmentsRequest(struct, apiVersion); | ||
| return ListPartitionReassignmentsRequest.parse(buffer, apiVersion); | ||
| case OFFSET_DELETE: | ||
| return new OffsetDeleteRequest(struct, apiVersion); | ||
| return OffsetDeleteRequest.parse(buffer, apiVersion); | ||
| case DESCRIBE_CLIENT_QUOTAS: | ||
| return new DescribeClientQuotasRequest(struct, apiVersion); | ||
| return DescribeClientQuotasRequest.parse(buffer, apiVersion); | ||
| case ALTER_CLIENT_QUOTAS: | ||
| return new AlterClientQuotasRequest(struct, apiVersion); | ||
| return AlterClientQuotasRequest.parse(buffer, apiVersion); | ||
| case DESCRIBE_USER_SCRAM_CREDENTIALS: | ||
| return new DescribeUserScramCredentialsRequest(struct, apiVersion); | ||
| return DescribeUserScramCredentialsRequest.parse(buffer, apiVersion); | ||
| case ALTER_USER_SCRAM_CREDENTIALS: | ||
| return new AlterUserScramCredentialsRequest(struct, apiVersion); | ||
| return AlterUserScramCredentialsRequest.parse(buffer, apiVersion); | ||
| case VOTE: | ||
| return new VoteRequest(struct, apiVersion); | ||
| return VoteRequest.parse(buffer, apiVersion); | ||
| case BEGIN_QUORUM_EPOCH: | ||
| return new BeginQuorumEpochRequest(struct, apiVersion); | ||
| return BeginQuorumEpochRequest.parse(buffer, apiVersion); | ||
| case END_QUORUM_EPOCH: | ||
| return new EndQuorumEpochRequest(struct, apiVersion); | ||
| return EndQuorumEpochRequest.parse(buffer, apiVersion); | ||
| case DESCRIBE_QUORUM: | ||
| return new DescribeQuorumRequest(struct, apiVersion); | ||
| return DescribeQuorumRequest.parse(buffer, apiVersion); | ||
| case ALTER_ISR: | ||
| return new AlterIsrRequest(new AlterIsrRequestData(struct, apiVersion), apiVersion); | ||
| return AlterIsrRequest.parse(buffer, apiVersion); | ||
| case UPDATE_FEATURES: | ||
| return new UpdateFeaturesRequest(struct, apiVersion); | ||
| return UpdateFeaturesRequest.parse(buffer, apiVersion); | ||
| case ENVELOPE: | ||
| return new EnvelopeRequest(struct, apiVersion); | ||
| return EnvelopeRequest.parse(buffer, apiVersion); | ||
| default: | ||
| throw new AssertionError(String.format("ApiKey %s is not currently handled in `parseRequest`, the " + | ||
| "code should be updated to do so.", apiKey)); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.