You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/12/14 15:42:35 UTC

[GitHub] [kafka] ijuma opened a new pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType and removing unused methods

ijuma opened a new pull request #9748:
URL: https://github.com/apache/kafka/pull/9748


   
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] chia7712 commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r543483040



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -289,49 +129,40 @@ public Struct parseResponse(short version, ByteBuffer buffer) {
     /** indicates whether the API is enabled for forwarding **/
     public final boolean forwardable;
 
-    public final Schema[] requestSchemas;
-    public final Schema[] responseSchemas;
     public final boolean requiresDelayedAllocation;
 
-    ApiKeys(int id, String name, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, false, requestSchemas, responseSchemas);
-    }
+    public final ApiMessageType messageType;
 
-    ApiKeys(int id, String name, boolean clusterAction, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, RecordBatch.MAGIC_VALUE_V0, requestSchemas, responseSchemas);
+    ApiKeys(ApiMessageType messageType) {
+        this(messageType, false);
     }
 
-    ApiKeys(int id, String name, Schema[] requestSchemas, Schema[] responseSchemas, boolean forwardable) {
-        this(id, name, false, RecordBatch.MAGIC_VALUE_V0, true, requestSchemas, responseSchemas, forwardable);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction) {
+        this(messageType, clusterAction, RecordBatch.MAGIC_VALUE_V0, false);
     }
 
-    ApiKeys(int id, String name, boolean clusterAction, boolean isEnabled, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, RecordBatch.MAGIC_VALUE_V0, isEnabled, requestSchemas, responseSchemas, false);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction, boolean forwardable) {
+        this(messageType, clusterAction, RecordBatch.MAGIC_VALUE_V0, forwardable);
     }
 
-    ApiKeys(int id, String name, boolean clusterAction, byte minRequiredInterBrokerMagic,
-            Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, minRequiredInterBrokerMagic, true, requestSchemas, responseSchemas, false);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction, byte minRequiredInterBrokerMagic, boolean forwardable) {
+        this(messageType, clusterAction, minRequiredInterBrokerMagic, forwardable, true);
     }
 
     ApiKeys(
-        int id,
-        String name,
+        ApiMessageType messageType,
         boolean clusterAction,
         byte minRequiredInterBrokerMagic,
-        boolean isEnabled,
-        Schema[] requestSchemas,
-        Schema[] responseSchemas,
-        boolean forwardable
+        boolean forwardable,
+        boolean isEnabled
     ) {
+        short id = messageType.apiKey();

Review comment:
       That alternative looks good to me :)
   
   




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] ijuma commented on pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType

Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#issuecomment-746383611


   JDK8 and 11 passed, 15 had one flaky unrelated failure:
   
   > kafka.server.ClientQuotasRequestTest.testAlterIpQuotasRequest


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] chia7712 commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r543804474



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -361,69 +172,35 @@ private static boolean shouldRetainsBufferReference(Schema[] requestSchemas) {
     }
 
     public static ApiKeys forId(int id) {

Review comment:
       Is 'short' type more suitable?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] chia7712 commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r543804854



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -361,69 +172,35 @@ private static boolean shouldRetainsBufferReference(Schema[] requestSchemas) {
     }
 
     public static ApiKeys forId(int id) {

Review comment:
       If the above comment is valid, it can be addressed in separate PR :)




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] chia7712 commented on pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType and removing unused methods

Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#issuecomment-745372032


   > Can you please elaborate? I don't see the assumption you mention. The main assumption is that api keys are dense. I changed the code not to rely on the latter although it doesn't change the behavior.
   
   I assumed the order of instancing ApiKeys is strict so removing the id may makes it hard to "see" where to put the new ApiKeys in the future. However, it seems the order of instancing ApiKeys is free so the worry is invalid. 


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] ijuma commented on pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType and removing unused methods

Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#issuecomment-745363005


   > As the "id" is removed from the construction, is it possible to add new ApiKeys instances with incorrect order in the future? the static field ID_TO_TYPE assume all ApiKeys instances are created by id order.
   
   @chia7712 Can you please elaborate? I don't see the assumption you mention. The main assumption is that api keys are dense. I changed the code not to rely on the latter although it doesn't change the behavior.


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] ijuma commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r543961009



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -157,124 +38,76 @@
  * Identifiers for all the Kafka APIs
  */
 public enum ApiKeys {
-    PRODUCE(0, "Produce", ProduceRequestData.SCHEMAS, ProduceResponseData.SCHEMAS),
-    FETCH(1, "Fetch", FetchRequestData.SCHEMAS, FetchResponseData.SCHEMAS),
-    LIST_OFFSETS(2, "ListOffsets", ListOffsetRequestData.SCHEMAS, ListOffsetResponseData.SCHEMAS),
-    METADATA(3, "Metadata", MetadataRequestData.SCHEMAS, MetadataResponseData.SCHEMAS),
-    LEADER_AND_ISR(4, "LeaderAndIsr", true, LeaderAndIsrRequestData.SCHEMAS, LeaderAndIsrResponseData.SCHEMAS),
-    STOP_REPLICA(5, "StopReplica", true, StopReplicaRequestData.SCHEMAS, StopReplicaResponseData.SCHEMAS),
-    UPDATE_METADATA(6, "UpdateMetadata", true, UpdateMetadataRequestData.SCHEMAS, UpdateMetadataResponseData.SCHEMAS),
-    CONTROLLED_SHUTDOWN(7, "ControlledShutdown", true, ControlledShutdownRequestData.SCHEMAS,
-            ControlledShutdownResponseData.SCHEMAS),
-    OFFSET_COMMIT(8, "OffsetCommit", OffsetCommitRequestData.SCHEMAS, OffsetCommitResponseData.SCHEMAS),
-    OFFSET_FETCH(9, "OffsetFetch", OffsetFetchRequestData.SCHEMAS, OffsetFetchResponseData.SCHEMAS),
-    FIND_COORDINATOR(10, "FindCoordinator", FindCoordinatorRequestData.SCHEMAS,
-            FindCoordinatorResponseData.SCHEMAS),
-    JOIN_GROUP(11, "JoinGroup", JoinGroupRequestData.SCHEMAS, JoinGroupResponseData.SCHEMAS),
-    HEARTBEAT(12, "Heartbeat", HeartbeatRequestData.SCHEMAS, HeartbeatResponseData.SCHEMAS),
-    LEAVE_GROUP(13, "LeaveGroup", LeaveGroupRequestData.SCHEMAS, LeaveGroupResponseData.SCHEMAS),
-    SYNC_GROUP(14, "SyncGroup", SyncGroupRequestData.SCHEMAS, SyncGroupResponseData.SCHEMAS),
-    DESCRIBE_GROUPS(15, "DescribeGroups", DescribeGroupsRequestData.SCHEMAS,
-            DescribeGroupsResponseData.SCHEMAS),
-    LIST_GROUPS(16, "ListGroups", ListGroupsRequestData.SCHEMAS, ListGroupsResponseData.SCHEMAS),
-    SASL_HANDSHAKE(17, "SaslHandshake", SaslHandshakeRequestData.SCHEMAS, SaslHandshakeResponseData.SCHEMAS),
-    API_VERSIONS(18, "ApiVersions", ApiVersionsRequestData.SCHEMAS, ApiVersionsResponseData.SCHEMAS) {
-        @Override
-        public Struct parseResponse(short version, ByteBuffer buffer) {
-            // Fallback to version 0 for ApiVersions response. If a client sends an ApiVersionsRequest
-            // using a version higher than that supported by the broker, a version 0 response is sent
-            // to the client indicating UNSUPPORTED_VERSION.
-            return parseResponse(version, buffer, (short) 0);
-        }
-    },
-    CREATE_TOPICS(19, "CreateTopics", CreateTopicsRequestData.SCHEMAS, CreateTopicsResponseData.SCHEMAS, true),
-    DELETE_TOPICS(20, "DeleteTopics", DeleteTopicsRequestData.SCHEMAS, DeleteTopicsResponseData.SCHEMAS, true),
-    DELETE_RECORDS(21, "DeleteRecords", DeleteRecordsRequestData.SCHEMAS, DeleteRecordsResponseData.SCHEMAS),
-    INIT_PRODUCER_ID(22, "InitProducerId", InitProducerIdRequestData.SCHEMAS, InitProducerIdResponseData.SCHEMAS),
-    OFFSET_FOR_LEADER_EPOCH(23, "OffsetForLeaderEpoch", false, OffsetForLeaderEpochRequestData.SCHEMAS,
-        OffsetForLeaderEpochResponseData.SCHEMAS),
-    ADD_PARTITIONS_TO_TXN(24, "AddPartitionsToTxn", false, RecordBatch.MAGIC_VALUE_V2,
-            AddPartitionsToTxnRequestData.SCHEMAS, AddPartitionsToTxnResponseData.SCHEMAS),
-    ADD_OFFSETS_TO_TXN(25, "AddOffsetsToTxn", false, RecordBatch.MAGIC_VALUE_V2, AddOffsetsToTxnRequestData.SCHEMAS,
-            AddOffsetsToTxnResponseData.SCHEMAS),
-    END_TXN(26, "EndTxn", false, RecordBatch.MAGIC_VALUE_V2, EndTxnRequestData.SCHEMAS, EndTxnResponseData.SCHEMAS),
-    WRITE_TXN_MARKERS(27, "WriteTxnMarkers", true, RecordBatch.MAGIC_VALUE_V2, WriteTxnMarkersRequestData.SCHEMAS,
-            WriteTxnMarkersResponseData.SCHEMAS),
-    TXN_OFFSET_COMMIT(28, "TxnOffsetCommit", false, RecordBatch.MAGIC_VALUE_V2, TxnOffsetCommitRequestData.SCHEMAS,
-            TxnOffsetCommitResponseData.SCHEMAS),
-    DESCRIBE_ACLS(29, "DescribeAcls", DescribeAclsRequestData.SCHEMAS, DescribeAclsResponseData.SCHEMAS),
-    CREATE_ACLS(30, "CreateAcls", CreateAclsRequestData.SCHEMAS, CreateAclsResponseData.SCHEMAS, true),
-    DELETE_ACLS(31, "DeleteAcls", DeleteAclsRequestData.SCHEMAS, DeleteAclsResponseData.SCHEMAS, true),
-    DESCRIBE_CONFIGS(32, "DescribeConfigs", DescribeConfigsRequestData.SCHEMAS,
-             DescribeConfigsResponseData.SCHEMAS),
-    ALTER_CONFIGS(33, "AlterConfigs", AlterConfigsRequestData.SCHEMAS,
-            AlterConfigsResponseData.SCHEMAS, true),
-    ALTER_REPLICA_LOG_DIRS(34, "AlterReplicaLogDirs", AlterReplicaLogDirsRequestData.SCHEMAS,
-            AlterReplicaLogDirsResponseData.SCHEMAS),
-    DESCRIBE_LOG_DIRS(35, "DescribeLogDirs", DescribeLogDirsRequestData.SCHEMAS,
-            DescribeLogDirsResponseData.SCHEMAS),
-    SASL_AUTHENTICATE(36, "SaslAuthenticate", SaslAuthenticateRequestData.SCHEMAS,
-            SaslAuthenticateResponseData.SCHEMAS),
-    CREATE_PARTITIONS(37, "CreatePartitions", CreatePartitionsRequestData.SCHEMAS,
-            CreatePartitionsResponseData.SCHEMAS, true),
-    CREATE_DELEGATION_TOKEN(38, "CreateDelegationToken", CreateDelegationTokenRequestData.SCHEMAS,
-            CreateDelegationTokenResponseData.SCHEMAS, true),
-    RENEW_DELEGATION_TOKEN(39, "RenewDelegationToken", RenewDelegationTokenRequestData.SCHEMAS,
-            RenewDelegationTokenResponseData.SCHEMAS, true),
-    EXPIRE_DELEGATION_TOKEN(40, "ExpireDelegationToken", ExpireDelegationTokenRequestData.SCHEMAS,
-            ExpireDelegationTokenResponseData.SCHEMAS, true),
-    DESCRIBE_DELEGATION_TOKEN(41, "DescribeDelegationToken", DescribeDelegationTokenRequestData.SCHEMAS,
-            DescribeDelegationTokenResponseData.SCHEMAS),
-    DELETE_GROUPS(42, "DeleteGroups", DeleteGroupsRequestData.SCHEMAS, DeleteGroupsResponseData.SCHEMAS),
-    ELECT_LEADERS(43, "ElectLeaders", ElectLeadersRequestData.SCHEMAS,
-            ElectLeadersResponseData.SCHEMAS),
-    INCREMENTAL_ALTER_CONFIGS(44, "IncrementalAlterConfigs", IncrementalAlterConfigsRequestData.SCHEMAS,
-            IncrementalAlterConfigsResponseData.SCHEMAS, true),
-    ALTER_PARTITION_REASSIGNMENTS(45, "AlterPartitionReassignments", AlterPartitionReassignmentsRequestData.SCHEMAS,
-            AlterPartitionReassignmentsResponseData.SCHEMAS, true),
-    LIST_PARTITION_REASSIGNMENTS(46, "ListPartitionReassignments", ListPartitionReassignmentsRequestData.SCHEMAS,
-            ListPartitionReassignmentsResponseData.SCHEMAS),
-    OFFSET_DELETE(47, "OffsetDelete", OffsetDeleteRequestData.SCHEMAS, OffsetDeleteResponseData.SCHEMAS),
-    DESCRIBE_CLIENT_QUOTAS(48, "DescribeClientQuotas", DescribeClientQuotasRequestData.SCHEMAS,
-            DescribeClientQuotasResponseData.SCHEMAS),
-    ALTER_CLIENT_QUOTAS(49, "AlterClientQuotas", AlterClientQuotasRequestData.SCHEMAS,
-            AlterClientQuotasResponseData.SCHEMAS, true),
-    DESCRIBE_USER_SCRAM_CREDENTIALS(50, "DescribeUserScramCredentials", DescribeUserScramCredentialsRequestData.SCHEMAS,
-            DescribeUserScramCredentialsResponseData.SCHEMAS),
-    ALTER_USER_SCRAM_CREDENTIALS(51, "AlterUserScramCredentials", AlterUserScramCredentialsRequestData.SCHEMAS,
-            AlterUserScramCredentialsResponseData.SCHEMAS, true),
-    VOTE(52, "Vote", true, false,
-        VoteRequestData.SCHEMAS, VoteResponseData.SCHEMAS),
-    BEGIN_QUORUM_EPOCH(53, "BeginQuorumEpoch", true, false,
-        BeginQuorumEpochRequestData.SCHEMAS, BeginQuorumEpochResponseData.SCHEMAS),
-    END_QUORUM_EPOCH(54, "EndQuorumEpoch", true, false,
-        EndQuorumEpochRequestData.SCHEMAS, EndQuorumEpochResponseData.SCHEMAS),
-    DESCRIBE_QUORUM(55, "DescribeQuorum", true, false,
-        DescribeQuorumRequestData.SCHEMAS, DescribeQuorumResponseData.SCHEMAS),
-    ALTER_ISR(56, "AlterIsr", true, AlterIsrRequestData.SCHEMAS, AlterIsrResponseData.SCHEMAS),
-    UPDATE_FEATURES(57, "UpdateFeatures",
-        UpdateFeaturesRequestData.SCHEMAS, UpdateFeaturesResponseData.SCHEMAS, true),
-    ENVELOPE(58, "Envelope", true, false, EnvelopeRequestData.SCHEMAS, EnvelopeResponseData.SCHEMAS);
-
-    private static final ApiKeys[] ID_TO_TYPE;
-    private static final int MIN_API_KEY = 0;
-    public static final int MAX_API_KEY;
-
+    PRODUCE(ApiMessageType.PRODUCE),
+    FETCH(ApiMessageType.FETCH),
+    LIST_OFFSETS(ApiMessageType.LIST_OFFSETS),
+    METADATA(ApiMessageType.METADATA),
+    LEADER_AND_ISR(ApiMessageType.LEADER_AND_ISR, true),
+    STOP_REPLICA(ApiMessageType.STOP_REPLICA, true),
+    UPDATE_METADATA(ApiMessageType.UPDATE_METADATA, true),
+    CONTROLLED_SHUTDOWN(ApiMessageType.CONTROLLED_SHUTDOWN, true),
+    OFFSET_COMMIT(ApiMessageType.OFFSET_COMMIT),
+    OFFSET_FETCH(ApiMessageType.OFFSET_FETCH),
+    FIND_COORDINATOR(ApiMessageType.FIND_COORDINATOR),
+    JOIN_GROUP(ApiMessageType.JOIN_GROUP),
+    HEARTBEAT(ApiMessageType.HEARTBEAT),
+    LEAVE_GROUP(ApiMessageType.LEAVE_GROUP),
+    SYNC_GROUP(ApiMessageType.SYNC_GROUP),
+    DESCRIBE_GROUPS(ApiMessageType.DESCRIBE_GROUPS),
+    LIST_GROUPS(ApiMessageType.LIST_GROUPS),
+    SASL_HANDSHAKE(ApiMessageType.SASL_HANDSHAKE),
+    API_VERSIONS(ApiMessageType.API_VERSIONS),
+    CREATE_TOPICS(ApiMessageType.CREATE_TOPICS, false, true),
+    DELETE_TOPICS(ApiMessageType.DELETE_TOPICS, false, true),
+    DELETE_RECORDS(ApiMessageType.DELETE_RECORDS),
+    INIT_PRODUCER_ID(ApiMessageType.INIT_PRODUCER_ID),
+    OFFSET_FOR_LEADER_EPOCH(ApiMessageType.OFFSET_FOR_LEADER_EPOCH),
+    ADD_PARTITIONS_TO_TXN(ApiMessageType.ADD_PARTITIONS_TO_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    ADD_OFFSETS_TO_TXN(ApiMessageType.ADD_OFFSETS_TO_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    END_TXN(ApiMessageType.END_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    WRITE_TXN_MARKERS(ApiMessageType.WRITE_TXN_MARKERS, true, RecordBatch.MAGIC_VALUE_V2, false),
+    TXN_OFFSET_COMMIT(ApiMessageType.TXN_OFFSET_COMMIT, false, RecordBatch.MAGIC_VALUE_V2, false),
+    DESCRIBE_ACLS(ApiMessageType.DESCRIBE_ACLS),
+    CREATE_ACLS(ApiMessageType.CREATE_ACLS, false, true),
+    DELETE_ACLS(ApiMessageType.DELETE_ACLS, false, true),
+    DESCRIBE_CONFIGS(ApiMessageType.DESCRIBE_CONFIGS),
+    ALTER_CONFIGS(ApiMessageType.ALTER_CONFIGS, false, true),
+    ALTER_REPLICA_LOG_DIRS(ApiMessageType.ALTER_REPLICA_LOG_DIRS),
+    DESCRIBE_LOG_DIRS(ApiMessageType.DESCRIBE_LOG_DIRS),
+    SASL_AUTHENTICATE(ApiMessageType.SASL_AUTHENTICATE),
+    CREATE_PARTITIONS(ApiMessageType.CREATE_PARTITIONS, false, true),
+    CREATE_DELEGATION_TOKEN(ApiMessageType.CREATE_DELEGATION_TOKEN, false, true),
+    RENEW_DELEGATION_TOKEN(ApiMessageType.RENEW_DELEGATION_TOKEN, false, true),
+    EXPIRE_DELEGATION_TOKEN(ApiMessageType.EXPIRE_DELEGATION_TOKEN, false, true),
+    DESCRIBE_DELEGATION_TOKEN(ApiMessageType.DESCRIBE_DELEGATION_TOKEN),
+    DELETE_GROUPS(ApiMessageType.DELETE_GROUPS),
+    ELECT_LEADERS(ApiMessageType.ELECT_LEADERS),
+    INCREMENTAL_ALTER_CONFIGS(ApiMessageType.INCREMENTAL_ALTER_CONFIGS, false, true),
+    ALTER_PARTITION_REASSIGNMENTS(ApiMessageType.ALTER_PARTITION_REASSIGNMENTS, false, true),
+    LIST_PARTITION_REASSIGNMENTS(ApiMessageType.LIST_PARTITION_REASSIGNMENTS),
+    OFFSET_DELETE(ApiMessageType.OFFSET_DELETE),
+    DESCRIBE_CLIENT_QUOTAS(ApiMessageType.DESCRIBE_CLIENT_QUOTAS),
+    ALTER_CLIENT_QUOTAS(ApiMessageType.ALTER_CLIENT_QUOTAS, false, true),
+    DESCRIBE_USER_SCRAM_CREDENTIALS(ApiMessageType.DESCRIBE_USER_SCRAM_CREDENTIALS),
+    ALTER_USER_SCRAM_CREDENTIALS(ApiMessageType.ALTER_USER_SCRAM_CREDENTIALS, false, true),
+    VOTE(ApiMessageType.VOTE, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    BEGIN_QUORUM_EPOCH(ApiMessageType.BEGIN_QUORUM_EPOCH, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    END_QUORUM_EPOCH(ApiMessageType.END_QUORUM_EPOCH, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    DESCRIBE_QUORUM(ApiMessageType.DESCRIBE_QUORUM, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    ALTER_ISR(ApiMessageType.ALTER_ISR, true),
+    UPDATE_FEATURES(ApiMessageType.UPDATE_FEATURES, false, true),
+    ENVELOPE(ApiMessageType.ENVELOPE, true, RecordBatch.MAGIC_VALUE_V0, false, false);
+
+    private static final Map<Integer, ApiKeys> ID_TO_TYPE = new HashMap<>();
     static {
-        int maxKey = -1;
-        for (ApiKeys key : ApiKeys.values())
-            maxKey = Math.max(maxKey, key.id);
-        ApiKeys[] idToType = new ApiKeys[maxKey + 1];
         for (ApiKeys key : ApiKeys.values())
-            idToType[key.id] = key;
-        ID_TO_TYPE = idToType;
-        MAX_API_KEY = maxKey;
+            ID_TO_TYPE.put((int) key.id, key);

Review comment:
       Yes, the generator checks it:
   
   > Caused by: java.lang.RuntimeException: Found more than one response with API key 3
   
   There's also `ApiMessageTypeTest.testUniqueness` that verifies it after the generation.
   
   I added a comment explaining that uniqueness is ensured.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] ijuma commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType and removing unused methods

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r543010324



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -289,49 +129,40 @@ public Struct parseResponse(short version, ByteBuffer buffer) {
     /** indicates whether the API is enabled for forwarding **/
     public final boolean forwardable;
 
-    public final Schema[] requestSchemas;
-    public final Schema[] responseSchemas;
     public final boolean requiresDelayedAllocation;
 
-    ApiKeys(int id, String name, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, false, requestSchemas, responseSchemas);
-    }
+    public final ApiMessageType messageType;
 
-    ApiKeys(int id, String name, boolean clusterAction, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, RecordBatch.MAGIC_VALUE_V0, requestSchemas, responseSchemas);
+    ApiKeys(ApiMessageType messageType) {
+        this(messageType, false);
     }
 
-    ApiKeys(int id, String name, Schema[] requestSchemas, Schema[] responseSchemas, boolean forwardable) {
-        this(id, name, false, RecordBatch.MAGIC_VALUE_V0, true, requestSchemas, responseSchemas, forwardable);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction) {
+        this(messageType, clusterAction, RecordBatch.MAGIC_VALUE_V0, false);
     }
 
-    ApiKeys(int id, String name, boolean clusterAction, boolean isEnabled, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, RecordBatch.MAGIC_VALUE_V0, isEnabled, requestSchemas, responseSchemas, false);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction, boolean forwardable) {
+        this(messageType, clusterAction, RecordBatch.MAGIC_VALUE_V0, forwardable);
     }
 
-    ApiKeys(int id, String name, boolean clusterAction, byte minRequiredInterBrokerMagic,
-            Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, minRequiredInterBrokerMagic, true, requestSchemas, responseSchemas, false);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction, byte minRequiredInterBrokerMagic, boolean forwardable) {
+        this(messageType, clusterAction, minRequiredInterBrokerMagic, forwardable, true);
     }
 
     ApiKeys(
-        int id,
-        String name,
+        ApiMessageType messageType,
         boolean clusterAction,
         byte minRequiredInterBrokerMagic,
-        boolean isEnabled,
-        Schema[] requestSchemas,
-        Schema[] responseSchemas,
-        boolean forwardable
+        boolean forwardable,
+        boolean isEnabled
     ) {
+        short id = messageType.apiKey();

Review comment:
       Oh, I see what you mean. You're suggesting to validate during generation, not to include the validation in the generated code.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] ijuma commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType and removing unused methods

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r542988497



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -289,49 +129,40 @@ public Struct parseResponse(short version, ByteBuffer buffer) {
     /** indicates whether the API is enabled for forwarding **/
     public final boolean forwardable;
 
-    public final Schema[] requestSchemas;
-    public final Schema[] responseSchemas;
     public final boolean requiresDelayedAllocation;
 
-    ApiKeys(int id, String name, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, false, requestSchemas, responseSchemas);
-    }
+    public final ApiMessageType messageType;
 
-    ApiKeys(int id, String name, boolean clusterAction, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, RecordBatch.MAGIC_VALUE_V0, requestSchemas, responseSchemas);
+    ApiKeys(ApiMessageType messageType) {
+        this(messageType, false);
     }
 
-    ApiKeys(int id, String name, Schema[] requestSchemas, Schema[] responseSchemas, boolean forwardable) {
-        this(id, name, false, RecordBatch.MAGIC_VALUE_V0, true, requestSchemas, responseSchemas, forwardable);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction) {
+        this(messageType, clusterAction, RecordBatch.MAGIC_VALUE_V0, false);
     }
 
-    ApiKeys(int id, String name, boolean clusterAction, boolean isEnabled, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, RecordBatch.MAGIC_VALUE_V0, isEnabled, requestSchemas, responseSchemas, false);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction, boolean forwardable) {
+        this(messageType, clusterAction, RecordBatch.MAGIC_VALUE_V0, forwardable);
     }
 
-    ApiKeys(int id, String name, boolean clusterAction, byte minRequiredInterBrokerMagic,
-            Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, minRequiredInterBrokerMagic, true, requestSchemas, responseSchemas, false);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction, byte minRequiredInterBrokerMagic, boolean forwardable) {
+        this(messageType, clusterAction, minRequiredInterBrokerMagic, forwardable, true);
     }
 
     ApiKeys(
-        int id,
-        String name,
+        ApiMessageType messageType,
         boolean clusterAction,
         byte minRequiredInterBrokerMagic,
-        boolean isEnabled,
-        Schema[] requestSchemas,
-        Schema[] responseSchemas,
-        boolean forwardable
+        boolean forwardable,
+        boolean isEnabled
     ) {
+        short id = messageType.apiKey();

Review comment:
       We can, but it doesn't seem particularly beneficial. It's important to generate code that is specific to each data class. Generic code is best written without generation.
   
   We can move this code to a base class for ApiMessageType potentially, particularly if we decide to remove ApiKeys.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] chia7712 commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r543799602



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -157,124 +38,76 @@
  * Identifiers for all the Kafka APIs
  */
 public enum ApiKeys {
-    PRODUCE(0, "Produce", ProduceRequestData.SCHEMAS, ProduceResponseData.SCHEMAS),
-    FETCH(1, "Fetch", FetchRequestData.SCHEMAS, FetchResponseData.SCHEMAS),
-    LIST_OFFSETS(2, "ListOffsets", ListOffsetRequestData.SCHEMAS, ListOffsetResponseData.SCHEMAS),
-    METADATA(3, "Metadata", MetadataRequestData.SCHEMAS, MetadataResponseData.SCHEMAS),
-    LEADER_AND_ISR(4, "LeaderAndIsr", true, LeaderAndIsrRequestData.SCHEMAS, LeaderAndIsrResponseData.SCHEMAS),
-    STOP_REPLICA(5, "StopReplica", true, StopReplicaRequestData.SCHEMAS, StopReplicaResponseData.SCHEMAS),
-    UPDATE_METADATA(6, "UpdateMetadata", true, UpdateMetadataRequestData.SCHEMAS, UpdateMetadataResponseData.SCHEMAS),
-    CONTROLLED_SHUTDOWN(7, "ControlledShutdown", true, ControlledShutdownRequestData.SCHEMAS,
-            ControlledShutdownResponseData.SCHEMAS),
-    OFFSET_COMMIT(8, "OffsetCommit", OffsetCommitRequestData.SCHEMAS, OffsetCommitResponseData.SCHEMAS),
-    OFFSET_FETCH(9, "OffsetFetch", OffsetFetchRequestData.SCHEMAS, OffsetFetchResponseData.SCHEMAS),
-    FIND_COORDINATOR(10, "FindCoordinator", FindCoordinatorRequestData.SCHEMAS,
-            FindCoordinatorResponseData.SCHEMAS),
-    JOIN_GROUP(11, "JoinGroup", JoinGroupRequestData.SCHEMAS, JoinGroupResponseData.SCHEMAS),
-    HEARTBEAT(12, "Heartbeat", HeartbeatRequestData.SCHEMAS, HeartbeatResponseData.SCHEMAS),
-    LEAVE_GROUP(13, "LeaveGroup", LeaveGroupRequestData.SCHEMAS, LeaveGroupResponseData.SCHEMAS),
-    SYNC_GROUP(14, "SyncGroup", SyncGroupRequestData.SCHEMAS, SyncGroupResponseData.SCHEMAS),
-    DESCRIBE_GROUPS(15, "DescribeGroups", DescribeGroupsRequestData.SCHEMAS,
-            DescribeGroupsResponseData.SCHEMAS),
-    LIST_GROUPS(16, "ListGroups", ListGroupsRequestData.SCHEMAS, ListGroupsResponseData.SCHEMAS),
-    SASL_HANDSHAKE(17, "SaslHandshake", SaslHandshakeRequestData.SCHEMAS, SaslHandshakeResponseData.SCHEMAS),
-    API_VERSIONS(18, "ApiVersions", ApiVersionsRequestData.SCHEMAS, ApiVersionsResponseData.SCHEMAS) {
-        @Override
-        public Struct parseResponse(short version, ByteBuffer buffer) {
-            // Fallback to version 0 for ApiVersions response. If a client sends an ApiVersionsRequest
-            // using a version higher than that supported by the broker, a version 0 response is sent
-            // to the client indicating UNSUPPORTED_VERSION.
-            return parseResponse(version, buffer, (short) 0);
-        }
-    },
-    CREATE_TOPICS(19, "CreateTopics", CreateTopicsRequestData.SCHEMAS, CreateTopicsResponseData.SCHEMAS, true),
-    DELETE_TOPICS(20, "DeleteTopics", DeleteTopicsRequestData.SCHEMAS, DeleteTopicsResponseData.SCHEMAS, true),
-    DELETE_RECORDS(21, "DeleteRecords", DeleteRecordsRequestData.SCHEMAS, DeleteRecordsResponseData.SCHEMAS),
-    INIT_PRODUCER_ID(22, "InitProducerId", InitProducerIdRequestData.SCHEMAS, InitProducerIdResponseData.SCHEMAS),
-    OFFSET_FOR_LEADER_EPOCH(23, "OffsetForLeaderEpoch", false, OffsetForLeaderEpochRequestData.SCHEMAS,
-        OffsetForLeaderEpochResponseData.SCHEMAS),
-    ADD_PARTITIONS_TO_TXN(24, "AddPartitionsToTxn", false, RecordBatch.MAGIC_VALUE_V2,
-            AddPartitionsToTxnRequestData.SCHEMAS, AddPartitionsToTxnResponseData.SCHEMAS),
-    ADD_OFFSETS_TO_TXN(25, "AddOffsetsToTxn", false, RecordBatch.MAGIC_VALUE_V2, AddOffsetsToTxnRequestData.SCHEMAS,
-            AddOffsetsToTxnResponseData.SCHEMAS),
-    END_TXN(26, "EndTxn", false, RecordBatch.MAGIC_VALUE_V2, EndTxnRequestData.SCHEMAS, EndTxnResponseData.SCHEMAS),
-    WRITE_TXN_MARKERS(27, "WriteTxnMarkers", true, RecordBatch.MAGIC_VALUE_V2, WriteTxnMarkersRequestData.SCHEMAS,
-            WriteTxnMarkersResponseData.SCHEMAS),
-    TXN_OFFSET_COMMIT(28, "TxnOffsetCommit", false, RecordBatch.MAGIC_VALUE_V2, TxnOffsetCommitRequestData.SCHEMAS,
-            TxnOffsetCommitResponseData.SCHEMAS),
-    DESCRIBE_ACLS(29, "DescribeAcls", DescribeAclsRequestData.SCHEMAS, DescribeAclsResponseData.SCHEMAS),
-    CREATE_ACLS(30, "CreateAcls", CreateAclsRequestData.SCHEMAS, CreateAclsResponseData.SCHEMAS, true),
-    DELETE_ACLS(31, "DeleteAcls", DeleteAclsRequestData.SCHEMAS, DeleteAclsResponseData.SCHEMAS, true),
-    DESCRIBE_CONFIGS(32, "DescribeConfigs", DescribeConfigsRequestData.SCHEMAS,
-             DescribeConfigsResponseData.SCHEMAS),
-    ALTER_CONFIGS(33, "AlterConfigs", AlterConfigsRequestData.SCHEMAS,
-            AlterConfigsResponseData.SCHEMAS, true),
-    ALTER_REPLICA_LOG_DIRS(34, "AlterReplicaLogDirs", AlterReplicaLogDirsRequestData.SCHEMAS,
-            AlterReplicaLogDirsResponseData.SCHEMAS),
-    DESCRIBE_LOG_DIRS(35, "DescribeLogDirs", DescribeLogDirsRequestData.SCHEMAS,
-            DescribeLogDirsResponseData.SCHEMAS),
-    SASL_AUTHENTICATE(36, "SaslAuthenticate", SaslAuthenticateRequestData.SCHEMAS,
-            SaslAuthenticateResponseData.SCHEMAS),
-    CREATE_PARTITIONS(37, "CreatePartitions", CreatePartitionsRequestData.SCHEMAS,
-            CreatePartitionsResponseData.SCHEMAS, true),
-    CREATE_DELEGATION_TOKEN(38, "CreateDelegationToken", CreateDelegationTokenRequestData.SCHEMAS,
-            CreateDelegationTokenResponseData.SCHEMAS, true),
-    RENEW_DELEGATION_TOKEN(39, "RenewDelegationToken", RenewDelegationTokenRequestData.SCHEMAS,
-            RenewDelegationTokenResponseData.SCHEMAS, true),
-    EXPIRE_DELEGATION_TOKEN(40, "ExpireDelegationToken", ExpireDelegationTokenRequestData.SCHEMAS,
-            ExpireDelegationTokenResponseData.SCHEMAS, true),
-    DESCRIBE_DELEGATION_TOKEN(41, "DescribeDelegationToken", DescribeDelegationTokenRequestData.SCHEMAS,
-            DescribeDelegationTokenResponseData.SCHEMAS),
-    DELETE_GROUPS(42, "DeleteGroups", DeleteGroupsRequestData.SCHEMAS, DeleteGroupsResponseData.SCHEMAS),
-    ELECT_LEADERS(43, "ElectLeaders", ElectLeadersRequestData.SCHEMAS,
-            ElectLeadersResponseData.SCHEMAS),
-    INCREMENTAL_ALTER_CONFIGS(44, "IncrementalAlterConfigs", IncrementalAlterConfigsRequestData.SCHEMAS,
-            IncrementalAlterConfigsResponseData.SCHEMAS, true),
-    ALTER_PARTITION_REASSIGNMENTS(45, "AlterPartitionReassignments", AlterPartitionReassignmentsRequestData.SCHEMAS,
-            AlterPartitionReassignmentsResponseData.SCHEMAS, true),
-    LIST_PARTITION_REASSIGNMENTS(46, "ListPartitionReassignments", ListPartitionReassignmentsRequestData.SCHEMAS,
-            ListPartitionReassignmentsResponseData.SCHEMAS),
-    OFFSET_DELETE(47, "OffsetDelete", OffsetDeleteRequestData.SCHEMAS, OffsetDeleteResponseData.SCHEMAS),
-    DESCRIBE_CLIENT_QUOTAS(48, "DescribeClientQuotas", DescribeClientQuotasRequestData.SCHEMAS,
-            DescribeClientQuotasResponseData.SCHEMAS),
-    ALTER_CLIENT_QUOTAS(49, "AlterClientQuotas", AlterClientQuotasRequestData.SCHEMAS,
-            AlterClientQuotasResponseData.SCHEMAS, true),
-    DESCRIBE_USER_SCRAM_CREDENTIALS(50, "DescribeUserScramCredentials", DescribeUserScramCredentialsRequestData.SCHEMAS,
-            DescribeUserScramCredentialsResponseData.SCHEMAS),
-    ALTER_USER_SCRAM_CREDENTIALS(51, "AlterUserScramCredentials", AlterUserScramCredentialsRequestData.SCHEMAS,
-            AlterUserScramCredentialsResponseData.SCHEMAS, true),
-    VOTE(52, "Vote", true, false,
-        VoteRequestData.SCHEMAS, VoteResponseData.SCHEMAS),
-    BEGIN_QUORUM_EPOCH(53, "BeginQuorumEpoch", true, false,
-        BeginQuorumEpochRequestData.SCHEMAS, BeginQuorumEpochResponseData.SCHEMAS),
-    END_QUORUM_EPOCH(54, "EndQuorumEpoch", true, false,
-        EndQuorumEpochRequestData.SCHEMAS, EndQuorumEpochResponseData.SCHEMAS),
-    DESCRIBE_QUORUM(55, "DescribeQuorum", true, false,
-        DescribeQuorumRequestData.SCHEMAS, DescribeQuorumResponseData.SCHEMAS),
-    ALTER_ISR(56, "AlterIsr", true, AlterIsrRequestData.SCHEMAS, AlterIsrResponseData.SCHEMAS),
-    UPDATE_FEATURES(57, "UpdateFeatures",
-        UpdateFeaturesRequestData.SCHEMAS, UpdateFeaturesResponseData.SCHEMAS, true),
-    ENVELOPE(58, "Envelope", true, false, EnvelopeRequestData.SCHEMAS, EnvelopeResponseData.SCHEMAS);
-
-    private static final ApiKeys[] ID_TO_TYPE;
-    private static final int MIN_API_KEY = 0;
-    public static final int MAX_API_KEY;
-
+    PRODUCE(ApiMessageType.PRODUCE),
+    FETCH(ApiMessageType.FETCH),
+    LIST_OFFSETS(ApiMessageType.LIST_OFFSETS),
+    METADATA(ApiMessageType.METADATA),
+    LEADER_AND_ISR(ApiMessageType.LEADER_AND_ISR, true),
+    STOP_REPLICA(ApiMessageType.STOP_REPLICA, true),
+    UPDATE_METADATA(ApiMessageType.UPDATE_METADATA, true),
+    CONTROLLED_SHUTDOWN(ApiMessageType.CONTROLLED_SHUTDOWN, true),
+    OFFSET_COMMIT(ApiMessageType.OFFSET_COMMIT),
+    OFFSET_FETCH(ApiMessageType.OFFSET_FETCH),
+    FIND_COORDINATOR(ApiMessageType.FIND_COORDINATOR),
+    JOIN_GROUP(ApiMessageType.JOIN_GROUP),
+    HEARTBEAT(ApiMessageType.HEARTBEAT),
+    LEAVE_GROUP(ApiMessageType.LEAVE_GROUP),
+    SYNC_GROUP(ApiMessageType.SYNC_GROUP),
+    DESCRIBE_GROUPS(ApiMessageType.DESCRIBE_GROUPS),
+    LIST_GROUPS(ApiMessageType.LIST_GROUPS),
+    SASL_HANDSHAKE(ApiMessageType.SASL_HANDSHAKE),
+    API_VERSIONS(ApiMessageType.API_VERSIONS),
+    CREATE_TOPICS(ApiMessageType.CREATE_TOPICS, false, true),
+    DELETE_TOPICS(ApiMessageType.DELETE_TOPICS, false, true),
+    DELETE_RECORDS(ApiMessageType.DELETE_RECORDS),
+    INIT_PRODUCER_ID(ApiMessageType.INIT_PRODUCER_ID),
+    OFFSET_FOR_LEADER_EPOCH(ApiMessageType.OFFSET_FOR_LEADER_EPOCH),
+    ADD_PARTITIONS_TO_TXN(ApiMessageType.ADD_PARTITIONS_TO_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    ADD_OFFSETS_TO_TXN(ApiMessageType.ADD_OFFSETS_TO_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    END_TXN(ApiMessageType.END_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    WRITE_TXN_MARKERS(ApiMessageType.WRITE_TXN_MARKERS, true, RecordBatch.MAGIC_VALUE_V2, false),
+    TXN_OFFSET_COMMIT(ApiMessageType.TXN_OFFSET_COMMIT, false, RecordBatch.MAGIC_VALUE_V2, false),
+    DESCRIBE_ACLS(ApiMessageType.DESCRIBE_ACLS),
+    CREATE_ACLS(ApiMessageType.CREATE_ACLS, false, true),
+    DELETE_ACLS(ApiMessageType.DELETE_ACLS, false, true),
+    DESCRIBE_CONFIGS(ApiMessageType.DESCRIBE_CONFIGS),
+    ALTER_CONFIGS(ApiMessageType.ALTER_CONFIGS, false, true),
+    ALTER_REPLICA_LOG_DIRS(ApiMessageType.ALTER_REPLICA_LOG_DIRS),
+    DESCRIBE_LOG_DIRS(ApiMessageType.DESCRIBE_LOG_DIRS),
+    SASL_AUTHENTICATE(ApiMessageType.SASL_AUTHENTICATE),
+    CREATE_PARTITIONS(ApiMessageType.CREATE_PARTITIONS, false, true),
+    CREATE_DELEGATION_TOKEN(ApiMessageType.CREATE_DELEGATION_TOKEN, false, true),
+    RENEW_DELEGATION_TOKEN(ApiMessageType.RENEW_DELEGATION_TOKEN, false, true),
+    EXPIRE_DELEGATION_TOKEN(ApiMessageType.EXPIRE_DELEGATION_TOKEN, false, true),
+    DESCRIBE_DELEGATION_TOKEN(ApiMessageType.DESCRIBE_DELEGATION_TOKEN),
+    DELETE_GROUPS(ApiMessageType.DELETE_GROUPS),
+    ELECT_LEADERS(ApiMessageType.ELECT_LEADERS),
+    INCREMENTAL_ALTER_CONFIGS(ApiMessageType.INCREMENTAL_ALTER_CONFIGS, false, true),
+    ALTER_PARTITION_REASSIGNMENTS(ApiMessageType.ALTER_PARTITION_REASSIGNMENTS, false, true),
+    LIST_PARTITION_REASSIGNMENTS(ApiMessageType.LIST_PARTITION_REASSIGNMENTS),
+    OFFSET_DELETE(ApiMessageType.OFFSET_DELETE),
+    DESCRIBE_CLIENT_QUOTAS(ApiMessageType.DESCRIBE_CLIENT_QUOTAS),
+    ALTER_CLIENT_QUOTAS(ApiMessageType.ALTER_CLIENT_QUOTAS, false, true),
+    DESCRIBE_USER_SCRAM_CREDENTIALS(ApiMessageType.DESCRIBE_USER_SCRAM_CREDENTIALS),
+    ALTER_USER_SCRAM_CREDENTIALS(ApiMessageType.ALTER_USER_SCRAM_CREDENTIALS, false, true),
+    VOTE(ApiMessageType.VOTE, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    BEGIN_QUORUM_EPOCH(ApiMessageType.BEGIN_QUORUM_EPOCH, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    END_QUORUM_EPOCH(ApiMessageType.END_QUORUM_EPOCH, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    DESCRIBE_QUORUM(ApiMessageType.DESCRIBE_QUORUM, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    ALTER_ISR(ApiMessageType.ALTER_ISR, true),
+    UPDATE_FEATURES(ApiMessageType.UPDATE_FEATURES, false, true),
+    ENVELOPE(ApiMessageType.ENVELOPE, true, RecordBatch.MAGIC_VALUE_V0, false, false);
+
+    private static final Map<Integer, ApiKeys> ID_TO_TYPE = new HashMap<>();
     static {
-        int maxKey = -1;
-        for (ApiKeys key : ApiKeys.values())
-            maxKey = Math.max(maxKey, key.id);
-        ApiKeys[] idToType = new ApiKeys[maxKey + 1];
         for (ApiKeys key : ApiKeys.values())
-            idToType[key.id] = key;
-        ID_TO_TYPE = idToType;
-        MAX_API_KEY = maxKey;
+            ID_TO_TYPE.put((int) key.id, key);

Review comment:
       Does it need to check duplicate id?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] chia7712 commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType and removing unused methods

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r542972073



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -289,49 +129,40 @@ public Struct parseResponse(short version, ByteBuffer buffer) {
     /** indicates whether the API is enabled for forwarding **/
     public final boolean forwardable;
 
-    public final Schema[] requestSchemas;
-    public final Schema[] responseSchemas;
     public final boolean requiresDelayedAllocation;
 
-    ApiKeys(int id, String name, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, false, requestSchemas, responseSchemas);
-    }
+    public final ApiMessageType messageType;
 
-    ApiKeys(int id, String name, boolean clusterAction, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, RecordBatch.MAGIC_VALUE_V0, requestSchemas, responseSchemas);
+    ApiKeys(ApiMessageType messageType) {
+        this(messageType, false);
     }
 
-    ApiKeys(int id, String name, Schema[] requestSchemas, Schema[] responseSchemas, boolean forwardable) {
-        this(id, name, false, RecordBatch.MAGIC_VALUE_V0, true, requestSchemas, responseSchemas, forwardable);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction) {
+        this(messageType, clusterAction, RecordBatch.MAGIC_VALUE_V0, false);
     }
 
-    ApiKeys(int id, String name, boolean clusterAction, boolean isEnabled, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, RecordBatch.MAGIC_VALUE_V0, isEnabled, requestSchemas, responseSchemas, false);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction, boolean forwardable) {
+        this(messageType, clusterAction, RecordBatch.MAGIC_VALUE_V0, forwardable);
     }
 
-    ApiKeys(int id, String name, boolean clusterAction, byte minRequiredInterBrokerMagic,
-            Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, minRequiredInterBrokerMagic, true, requestSchemas, responseSchemas, false);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction, byte minRequiredInterBrokerMagic, boolean forwardable) {
+        this(messageType, clusterAction, minRequiredInterBrokerMagic, forwardable, true);
     }
 
     ApiKeys(
-        int id,
-        String name,
+        ApiMessageType messageType,
         boolean clusterAction,
         byte minRequiredInterBrokerMagic,
-        boolean isEnabled,
-        Schema[] requestSchemas,
-        Schema[] responseSchemas,
-        boolean forwardable
+        boolean forwardable,
+        boolean isEnabled
     ) {
+        short id = messageType.apiKey();

Review comment:
       It seems to me this check should be moved to ```ApiMessageTypeGenerator``` as it is a generated code from json.

##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -289,49 +129,40 @@ public Struct parseResponse(short version, ByteBuffer buffer) {
     /** indicates whether the API is enabled for forwarding **/
     public final boolean forwardable;
 
-    public final Schema[] requestSchemas;
-    public final Schema[] responseSchemas;
     public final boolean requiresDelayedAllocation;
 
-    ApiKeys(int id, String name, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, false, requestSchemas, responseSchemas);
-    }
+    public final ApiMessageType messageType;
 
-    ApiKeys(int id, String name, boolean clusterAction, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, RecordBatch.MAGIC_VALUE_V0, requestSchemas, responseSchemas);
+    ApiKeys(ApiMessageType messageType) {
+        this(messageType, false);
     }
 
-    ApiKeys(int id, String name, Schema[] requestSchemas, Schema[] responseSchemas, boolean forwardable) {
-        this(id, name, false, RecordBatch.MAGIC_VALUE_V0, true, requestSchemas, responseSchemas, forwardable);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction) {
+        this(messageType, clusterAction, RecordBatch.MAGIC_VALUE_V0, false);
     }
 
-    ApiKeys(int id, String name, boolean clusterAction, boolean isEnabled, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, RecordBatch.MAGIC_VALUE_V0, isEnabled, requestSchemas, responseSchemas, false);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction, boolean forwardable) {
+        this(messageType, clusterAction, RecordBatch.MAGIC_VALUE_V0, forwardable);
     }
 
-    ApiKeys(int id, String name, boolean clusterAction, byte minRequiredInterBrokerMagic,
-            Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, minRequiredInterBrokerMagic, true, requestSchemas, responseSchemas, false);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction, byte minRequiredInterBrokerMagic, boolean forwardable) {
+        this(messageType, clusterAction, minRequiredInterBrokerMagic, forwardable, true);
     }
 
     ApiKeys(
-        int id,
-        String name,
+        ApiMessageType messageType,
         boolean clusterAction,
         byte minRequiredInterBrokerMagic,
-        boolean isEnabled,
-        Schema[] requestSchemas,
-        Schema[] responseSchemas,
-        boolean forwardable
+        boolean forwardable,
+        boolean isEnabled
     ) {
+        short id = messageType.apiKey();
         if (id < 0)
             throw new IllegalArgumentException("id must not be negative, id: " + id);
-        this.id = (short) id;
-        this.name = name;
-        this.clusterAction = clusterAction;
-        this.minRequiredInterBrokerMagic = minRequiredInterBrokerMagic;
-        this.isEnabled = isEnabled;
 
+        Schema[] requestSchemas = messageType.requestSchemas();
+        Schema[] responseSchemas = messageType.responseSchemas();
+        String name = messageType.name;
         if (requestSchemas.length != responseSchemas.length)

Review comment:
       ditto




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] ijuma merged pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType

Posted by GitBox <gi...@apache.org>.
ijuma merged pull request #9748:
URL: https://github.com/apache/kafka/pull/9748


   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] ijuma commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r543815467



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -157,124 +38,76 @@
  * Identifiers for all the Kafka APIs
  */
 public enum ApiKeys {
-    PRODUCE(0, "Produce", ProduceRequestData.SCHEMAS, ProduceResponseData.SCHEMAS),
-    FETCH(1, "Fetch", FetchRequestData.SCHEMAS, FetchResponseData.SCHEMAS),
-    LIST_OFFSETS(2, "ListOffsets", ListOffsetRequestData.SCHEMAS, ListOffsetResponseData.SCHEMAS),
-    METADATA(3, "Metadata", MetadataRequestData.SCHEMAS, MetadataResponseData.SCHEMAS),
-    LEADER_AND_ISR(4, "LeaderAndIsr", true, LeaderAndIsrRequestData.SCHEMAS, LeaderAndIsrResponseData.SCHEMAS),
-    STOP_REPLICA(5, "StopReplica", true, StopReplicaRequestData.SCHEMAS, StopReplicaResponseData.SCHEMAS),
-    UPDATE_METADATA(6, "UpdateMetadata", true, UpdateMetadataRequestData.SCHEMAS, UpdateMetadataResponseData.SCHEMAS),
-    CONTROLLED_SHUTDOWN(7, "ControlledShutdown", true, ControlledShutdownRequestData.SCHEMAS,
-            ControlledShutdownResponseData.SCHEMAS),
-    OFFSET_COMMIT(8, "OffsetCommit", OffsetCommitRequestData.SCHEMAS, OffsetCommitResponseData.SCHEMAS),
-    OFFSET_FETCH(9, "OffsetFetch", OffsetFetchRequestData.SCHEMAS, OffsetFetchResponseData.SCHEMAS),
-    FIND_COORDINATOR(10, "FindCoordinator", FindCoordinatorRequestData.SCHEMAS,
-            FindCoordinatorResponseData.SCHEMAS),
-    JOIN_GROUP(11, "JoinGroup", JoinGroupRequestData.SCHEMAS, JoinGroupResponseData.SCHEMAS),
-    HEARTBEAT(12, "Heartbeat", HeartbeatRequestData.SCHEMAS, HeartbeatResponseData.SCHEMAS),
-    LEAVE_GROUP(13, "LeaveGroup", LeaveGroupRequestData.SCHEMAS, LeaveGroupResponseData.SCHEMAS),
-    SYNC_GROUP(14, "SyncGroup", SyncGroupRequestData.SCHEMAS, SyncGroupResponseData.SCHEMAS),
-    DESCRIBE_GROUPS(15, "DescribeGroups", DescribeGroupsRequestData.SCHEMAS,
-            DescribeGroupsResponseData.SCHEMAS),
-    LIST_GROUPS(16, "ListGroups", ListGroupsRequestData.SCHEMAS, ListGroupsResponseData.SCHEMAS),
-    SASL_HANDSHAKE(17, "SaslHandshake", SaslHandshakeRequestData.SCHEMAS, SaslHandshakeResponseData.SCHEMAS),
-    API_VERSIONS(18, "ApiVersions", ApiVersionsRequestData.SCHEMAS, ApiVersionsResponseData.SCHEMAS) {
-        @Override
-        public Struct parseResponse(short version, ByteBuffer buffer) {
-            // Fallback to version 0 for ApiVersions response. If a client sends an ApiVersionsRequest
-            // using a version higher than that supported by the broker, a version 0 response is sent
-            // to the client indicating UNSUPPORTED_VERSION.
-            return parseResponse(version, buffer, (short) 0);
-        }
-    },
-    CREATE_TOPICS(19, "CreateTopics", CreateTopicsRequestData.SCHEMAS, CreateTopicsResponseData.SCHEMAS, true),
-    DELETE_TOPICS(20, "DeleteTopics", DeleteTopicsRequestData.SCHEMAS, DeleteTopicsResponseData.SCHEMAS, true),
-    DELETE_RECORDS(21, "DeleteRecords", DeleteRecordsRequestData.SCHEMAS, DeleteRecordsResponseData.SCHEMAS),
-    INIT_PRODUCER_ID(22, "InitProducerId", InitProducerIdRequestData.SCHEMAS, InitProducerIdResponseData.SCHEMAS),
-    OFFSET_FOR_LEADER_EPOCH(23, "OffsetForLeaderEpoch", false, OffsetForLeaderEpochRequestData.SCHEMAS,
-        OffsetForLeaderEpochResponseData.SCHEMAS),
-    ADD_PARTITIONS_TO_TXN(24, "AddPartitionsToTxn", false, RecordBatch.MAGIC_VALUE_V2,
-            AddPartitionsToTxnRequestData.SCHEMAS, AddPartitionsToTxnResponseData.SCHEMAS),
-    ADD_OFFSETS_TO_TXN(25, "AddOffsetsToTxn", false, RecordBatch.MAGIC_VALUE_V2, AddOffsetsToTxnRequestData.SCHEMAS,
-            AddOffsetsToTxnResponseData.SCHEMAS),
-    END_TXN(26, "EndTxn", false, RecordBatch.MAGIC_VALUE_V2, EndTxnRequestData.SCHEMAS, EndTxnResponseData.SCHEMAS),
-    WRITE_TXN_MARKERS(27, "WriteTxnMarkers", true, RecordBatch.MAGIC_VALUE_V2, WriteTxnMarkersRequestData.SCHEMAS,
-            WriteTxnMarkersResponseData.SCHEMAS),
-    TXN_OFFSET_COMMIT(28, "TxnOffsetCommit", false, RecordBatch.MAGIC_VALUE_V2, TxnOffsetCommitRequestData.SCHEMAS,
-            TxnOffsetCommitResponseData.SCHEMAS),
-    DESCRIBE_ACLS(29, "DescribeAcls", DescribeAclsRequestData.SCHEMAS, DescribeAclsResponseData.SCHEMAS),
-    CREATE_ACLS(30, "CreateAcls", CreateAclsRequestData.SCHEMAS, CreateAclsResponseData.SCHEMAS, true),
-    DELETE_ACLS(31, "DeleteAcls", DeleteAclsRequestData.SCHEMAS, DeleteAclsResponseData.SCHEMAS, true),
-    DESCRIBE_CONFIGS(32, "DescribeConfigs", DescribeConfigsRequestData.SCHEMAS,
-             DescribeConfigsResponseData.SCHEMAS),
-    ALTER_CONFIGS(33, "AlterConfigs", AlterConfigsRequestData.SCHEMAS,
-            AlterConfigsResponseData.SCHEMAS, true),
-    ALTER_REPLICA_LOG_DIRS(34, "AlterReplicaLogDirs", AlterReplicaLogDirsRequestData.SCHEMAS,
-            AlterReplicaLogDirsResponseData.SCHEMAS),
-    DESCRIBE_LOG_DIRS(35, "DescribeLogDirs", DescribeLogDirsRequestData.SCHEMAS,
-            DescribeLogDirsResponseData.SCHEMAS),
-    SASL_AUTHENTICATE(36, "SaslAuthenticate", SaslAuthenticateRequestData.SCHEMAS,
-            SaslAuthenticateResponseData.SCHEMAS),
-    CREATE_PARTITIONS(37, "CreatePartitions", CreatePartitionsRequestData.SCHEMAS,
-            CreatePartitionsResponseData.SCHEMAS, true),
-    CREATE_DELEGATION_TOKEN(38, "CreateDelegationToken", CreateDelegationTokenRequestData.SCHEMAS,
-            CreateDelegationTokenResponseData.SCHEMAS, true),
-    RENEW_DELEGATION_TOKEN(39, "RenewDelegationToken", RenewDelegationTokenRequestData.SCHEMAS,
-            RenewDelegationTokenResponseData.SCHEMAS, true),
-    EXPIRE_DELEGATION_TOKEN(40, "ExpireDelegationToken", ExpireDelegationTokenRequestData.SCHEMAS,
-            ExpireDelegationTokenResponseData.SCHEMAS, true),
-    DESCRIBE_DELEGATION_TOKEN(41, "DescribeDelegationToken", DescribeDelegationTokenRequestData.SCHEMAS,
-            DescribeDelegationTokenResponseData.SCHEMAS),
-    DELETE_GROUPS(42, "DeleteGroups", DeleteGroupsRequestData.SCHEMAS, DeleteGroupsResponseData.SCHEMAS),
-    ELECT_LEADERS(43, "ElectLeaders", ElectLeadersRequestData.SCHEMAS,
-            ElectLeadersResponseData.SCHEMAS),
-    INCREMENTAL_ALTER_CONFIGS(44, "IncrementalAlterConfigs", IncrementalAlterConfigsRequestData.SCHEMAS,
-            IncrementalAlterConfigsResponseData.SCHEMAS, true),
-    ALTER_PARTITION_REASSIGNMENTS(45, "AlterPartitionReassignments", AlterPartitionReassignmentsRequestData.SCHEMAS,
-            AlterPartitionReassignmentsResponseData.SCHEMAS, true),
-    LIST_PARTITION_REASSIGNMENTS(46, "ListPartitionReassignments", ListPartitionReassignmentsRequestData.SCHEMAS,
-            ListPartitionReassignmentsResponseData.SCHEMAS),
-    OFFSET_DELETE(47, "OffsetDelete", OffsetDeleteRequestData.SCHEMAS, OffsetDeleteResponseData.SCHEMAS),
-    DESCRIBE_CLIENT_QUOTAS(48, "DescribeClientQuotas", DescribeClientQuotasRequestData.SCHEMAS,
-            DescribeClientQuotasResponseData.SCHEMAS),
-    ALTER_CLIENT_QUOTAS(49, "AlterClientQuotas", AlterClientQuotasRequestData.SCHEMAS,
-            AlterClientQuotasResponseData.SCHEMAS, true),
-    DESCRIBE_USER_SCRAM_CREDENTIALS(50, "DescribeUserScramCredentials", DescribeUserScramCredentialsRequestData.SCHEMAS,
-            DescribeUserScramCredentialsResponseData.SCHEMAS),
-    ALTER_USER_SCRAM_CREDENTIALS(51, "AlterUserScramCredentials", AlterUserScramCredentialsRequestData.SCHEMAS,
-            AlterUserScramCredentialsResponseData.SCHEMAS, true),
-    VOTE(52, "Vote", true, false,
-        VoteRequestData.SCHEMAS, VoteResponseData.SCHEMAS),
-    BEGIN_QUORUM_EPOCH(53, "BeginQuorumEpoch", true, false,
-        BeginQuorumEpochRequestData.SCHEMAS, BeginQuorumEpochResponseData.SCHEMAS),
-    END_QUORUM_EPOCH(54, "EndQuorumEpoch", true, false,
-        EndQuorumEpochRequestData.SCHEMAS, EndQuorumEpochResponseData.SCHEMAS),
-    DESCRIBE_QUORUM(55, "DescribeQuorum", true, false,
-        DescribeQuorumRequestData.SCHEMAS, DescribeQuorumResponseData.SCHEMAS),
-    ALTER_ISR(56, "AlterIsr", true, AlterIsrRequestData.SCHEMAS, AlterIsrResponseData.SCHEMAS),
-    UPDATE_FEATURES(57, "UpdateFeatures",
-        UpdateFeaturesRequestData.SCHEMAS, UpdateFeaturesResponseData.SCHEMAS, true),
-    ENVELOPE(58, "Envelope", true, false, EnvelopeRequestData.SCHEMAS, EnvelopeResponseData.SCHEMAS);
-
-    private static final ApiKeys[] ID_TO_TYPE;
-    private static final int MIN_API_KEY = 0;
-    public static final int MAX_API_KEY;
-
+    PRODUCE(ApiMessageType.PRODUCE),
+    FETCH(ApiMessageType.FETCH),
+    LIST_OFFSETS(ApiMessageType.LIST_OFFSETS),
+    METADATA(ApiMessageType.METADATA),
+    LEADER_AND_ISR(ApiMessageType.LEADER_AND_ISR, true),
+    STOP_REPLICA(ApiMessageType.STOP_REPLICA, true),
+    UPDATE_METADATA(ApiMessageType.UPDATE_METADATA, true),
+    CONTROLLED_SHUTDOWN(ApiMessageType.CONTROLLED_SHUTDOWN, true),
+    OFFSET_COMMIT(ApiMessageType.OFFSET_COMMIT),
+    OFFSET_FETCH(ApiMessageType.OFFSET_FETCH),
+    FIND_COORDINATOR(ApiMessageType.FIND_COORDINATOR),
+    JOIN_GROUP(ApiMessageType.JOIN_GROUP),
+    HEARTBEAT(ApiMessageType.HEARTBEAT),
+    LEAVE_GROUP(ApiMessageType.LEAVE_GROUP),
+    SYNC_GROUP(ApiMessageType.SYNC_GROUP),
+    DESCRIBE_GROUPS(ApiMessageType.DESCRIBE_GROUPS),
+    LIST_GROUPS(ApiMessageType.LIST_GROUPS),
+    SASL_HANDSHAKE(ApiMessageType.SASL_HANDSHAKE),
+    API_VERSIONS(ApiMessageType.API_VERSIONS),
+    CREATE_TOPICS(ApiMessageType.CREATE_TOPICS, false, true),
+    DELETE_TOPICS(ApiMessageType.DELETE_TOPICS, false, true),
+    DELETE_RECORDS(ApiMessageType.DELETE_RECORDS),
+    INIT_PRODUCER_ID(ApiMessageType.INIT_PRODUCER_ID),
+    OFFSET_FOR_LEADER_EPOCH(ApiMessageType.OFFSET_FOR_LEADER_EPOCH),
+    ADD_PARTITIONS_TO_TXN(ApiMessageType.ADD_PARTITIONS_TO_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    ADD_OFFSETS_TO_TXN(ApiMessageType.ADD_OFFSETS_TO_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    END_TXN(ApiMessageType.END_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    WRITE_TXN_MARKERS(ApiMessageType.WRITE_TXN_MARKERS, true, RecordBatch.MAGIC_VALUE_V2, false),
+    TXN_OFFSET_COMMIT(ApiMessageType.TXN_OFFSET_COMMIT, false, RecordBatch.MAGIC_VALUE_V2, false),
+    DESCRIBE_ACLS(ApiMessageType.DESCRIBE_ACLS),
+    CREATE_ACLS(ApiMessageType.CREATE_ACLS, false, true),
+    DELETE_ACLS(ApiMessageType.DELETE_ACLS, false, true),
+    DESCRIBE_CONFIGS(ApiMessageType.DESCRIBE_CONFIGS),
+    ALTER_CONFIGS(ApiMessageType.ALTER_CONFIGS, false, true),
+    ALTER_REPLICA_LOG_DIRS(ApiMessageType.ALTER_REPLICA_LOG_DIRS),
+    DESCRIBE_LOG_DIRS(ApiMessageType.DESCRIBE_LOG_DIRS),
+    SASL_AUTHENTICATE(ApiMessageType.SASL_AUTHENTICATE),
+    CREATE_PARTITIONS(ApiMessageType.CREATE_PARTITIONS, false, true),
+    CREATE_DELEGATION_TOKEN(ApiMessageType.CREATE_DELEGATION_TOKEN, false, true),
+    RENEW_DELEGATION_TOKEN(ApiMessageType.RENEW_DELEGATION_TOKEN, false, true),
+    EXPIRE_DELEGATION_TOKEN(ApiMessageType.EXPIRE_DELEGATION_TOKEN, false, true),
+    DESCRIBE_DELEGATION_TOKEN(ApiMessageType.DESCRIBE_DELEGATION_TOKEN),
+    DELETE_GROUPS(ApiMessageType.DELETE_GROUPS),
+    ELECT_LEADERS(ApiMessageType.ELECT_LEADERS),
+    INCREMENTAL_ALTER_CONFIGS(ApiMessageType.INCREMENTAL_ALTER_CONFIGS, false, true),
+    ALTER_PARTITION_REASSIGNMENTS(ApiMessageType.ALTER_PARTITION_REASSIGNMENTS, false, true),
+    LIST_PARTITION_REASSIGNMENTS(ApiMessageType.LIST_PARTITION_REASSIGNMENTS),
+    OFFSET_DELETE(ApiMessageType.OFFSET_DELETE),
+    DESCRIBE_CLIENT_QUOTAS(ApiMessageType.DESCRIBE_CLIENT_QUOTAS),
+    ALTER_CLIENT_QUOTAS(ApiMessageType.ALTER_CLIENT_QUOTAS, false, true),
+    DESCRIBE_USER_SCRAM_CREDENTIALS(ApiMessageType.DESCRIBE_USER_SCRAM_CREDENTIALS),
+    ALTER_USER_SCRAM_CREDENTIALS(ApiMessageType.ALTER_USER_SCRAM_CREDENTIALS, false, true),
+    VOTE(ApiMessageType.VOTE, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    BEGIN_QUORUM_EPOCH(ApiMessageType.BEGIN_QUORUM_EPOCH, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    END_QUORUM_EPOCH(ApiMessageType.END_QUORUM_EPOCH, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    DESCRIBE_QUORUM(ApiMessageType.DESCRIBE_QUORUM, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    ALTER_ISR(ApiMessageType.ALTER_ISR, true),
+    UPDATE_FEATURES(ApiMessageType.UPDATE_FEATURES, false, true),
+    ENVELOPE(ApiMessageType.ENVELOPE, true, RecordBatch.MAGIC_VALUE_V0, false, false);
+
+    private static final Map<Integer, ApiKeys> ID_TO_TYPE = new HashMap<>();
     static {
-        int maxKey = -1;
-        for (ApiKeys key : ApiKeys.values())
-            maxKey = Math.max(maxKey, key.id);
-        ApiKeys[] idToType = new ApiKeys[maxKey + 1];
         for (ApiKeys key : ApiKeys.values())
-            idToType[key.id] = key;
-        ID_TO_TYPE = idToType;
-        MAX_API_KEY = maxKey;
+            ID_TO_TYPE.put((int) key.id, key);

Review comment:
       I think the generator checks for this. I will double check.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] ijuma commented on pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType

Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#issuecomment-745618466


   Tests passed for JDK 11 and 15. JDK 8 had one flaky failure:
   
   > kafka.api.ConsumerBounceTest.testClose failed
   
   @chia7712 Do you have cycles to review this?


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] ijuma commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType and removing unused methods

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r543472687



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -289,49 +129,40 @@ public Struct parseResponse(short version, ByteBuffer buffer) {
     /** indicates whether the API is enabled for forwarding **/
     public final boolean forwardable;
 
-    public final Schema[] requestSchemas;
-    public final Schema[] responseSchemas;
     public final boolean requiresDelayedAllocation;
 
-    ApiKeys(int id, String name, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, false, requestSchemas, responseSchemas);
-    }
+    public final ApiMessageType messageType;
 
-    ApiKeys(int id, String name, boolean clusterAction, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, RecordBatch.MAGIC_VALUE_V0, requestSchemas, responseSchemas);
+    ApiKeys(ApiMessageType messageType) {
+        this(messageType, false);
     }
 
-    ApiKeys(int id, String name, Schema[] requestSchemas, Schema[] responseSchemas, boolean forwardable) {
-        this(id, name, false, RecordBatch.MAGIC_VALUE_V0, true, requestSchemas, responseSchemas, forwardable);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction) {
+        this(messageType, clusterAction, RecordBatch.MAGIC_VALUE_V0, false);
     }
 
-    ApiKeys(int id, String name, boolean clusterAction, boolean isEnabled, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, RecordBatch.MAGIC_VALUE_V0, isEnabled, requestSchemas, responseSchemas, false);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction, boolean forwardable) {
+        this(messageType, clusterAction, RecordBatch.MAGIC_VALUE_V0, forwardable);
     }
 
-    ApiKeys(int id, String name, boolean clusterAction, byte minRequiredInterBrokerMagic,
-            Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, minRequiredInterBrokerMagic, true, requestSchemas, responseSchemas, false);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction, byte minRequiredInterBrokerMagic, boolean forwardable) {
+        this(messageType, clusterAction, minRequiredInterBrokerMagic, forwardable, true);
     }
 
     ApiKeys(
-        int id,
-        String name,
+        ApiMessageType messageType,
         boolean clusterAction,
         byte minRequiredInterBrokerMagic,
-        boolean isEnabled,
-        Schema[] requestSchemas,
-        Schema[] responseSchemas,
-        boolean forwardable
+        boolean forwardable,
+        boolean isEnabled
     ) {
+        short id = messageType.apiKey();

Review comment:
       I took a look at this and not clear how to validate this in the generator. I added unit tests to `ApiMessageTypeTest` instead and I think that's good enough for now. What do you think?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] chia7712 commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType and removing unused methods

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r542999763



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -289,49 +129,40 @@ public Struct parseResponse(short version, ByteBuffer buffer) {
     /** indicates whether the API is enabled for forwarding **/
     public final boolean forwardable;
 
-    public final Schema[] requestSchemas;
-    public final Schema[] responseSchemas;
     public final boolean requiresDelayedAllocation;
 
-    ApiKeys(int id, String name, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, false, requestSchemas, responseSchemas);
-    }
+    public final ApiMessageType messageType;
 
-    ApiKeys(int id, String name, boolean clusterAction, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, RecordBatch.MAGIC_VALUE_V0, requestSchemas, responseSchemas);
+    ApiKeys(ApiMessageType messageType) {
+        this(messageType, false);
     }
 
-    ApiKeys(int id, String name, Schema[] requestSchemas, Schema[] responseSchemas, boolean forwardable) {
-        this(id, name, false, RecordBatch.MAGIC_VALUE_V0, true, requestSchemas, responseSchemas, forwardable);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction) {
+        this(messageType, clusterAction, RecordBatch.MAGIC_VALUE_V0, false);
     }
 
-    ApiKeys(int id, String name, boolean clusterAction, boolean isEnabled, Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, RecordBatch.MAGIC_VALUE_V0, isEnabled, requestSchemas, responseSchemas, false);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction, boolean forwardable) {
+        this(messageType, clusterAction, RecordBatch.MAGIC_VALUE_V0, forwardable);
     }
 
-    ApiKeys(int id, String name, boolean clusterAction, byte minRequiredInterBrokerMagic,
-            Schema[] requestSchemas, Schema[] responseSchemas) {
-        this(id, name, clusterAction, minRequiredInterBrokerMagic, true, requestSchemas, responseSchemas, false);
+    ApiKeys(ApiMessageType messageType, boolean clusterAction, byte minRequiredInterBrokerMagic, boolean forwardable) {
+        this(messageType, clusterAction, minRequiredInterBrokerMagic, forwardable, true);
     }
 
     ApiKeys(
-        int id,
-        String name,
+        ApiMessageType messageType,
         boolean clusterAction,
         byte minRequiredInterBrokerMagic,
-        boolean isEnabled,
-        Schema[] requestSchemas,
-        Schema[] responseSchemas,
-        boolean forwardable
+        boolean forwardable,
+        boolean isEnabled
     ) {
+        short id = messageType.apiKey();

Review comment:
       >  it doesn't seem particularly beneficial
   
   It seems to me the benefit is the error happens from runtime/test_runtime to build time (generate message code).




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] chia7712 commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType and removing unused methods

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r542962306



##########
File path: generator/src/main/java/org/apache/kafka/message/ApiMessageTypeGenerator.java
##########
@@ -169,7 +169,7 @@ private void generateEnumValues() {
     }
 
     private void generateInstanceVariables() {
-        buffer.printf("private final String name;%n");
+        buffer.printf("public final String name;%n");

Review comment:
       It is a bit weird that ```messageType.name``` and ```messageType.name()``` return different string.

##########
File path: clients/src/test/java/org/apache/kafka/common/message/MessageTest.java
##########
@@ -952,40 +904,6 @@ public String toString() {
         }
     }
 
-    private static void compareTypes(Schema schemaA, Schema schemaB) {
-        compareTypes(new NamedType("schemaA", schemaA),
-                     new NamedType("schemaB", schemaB));
-    }
-
-    private static void compareTypes(NamedType typeA, NamedType typeB) {
-        List<NamedType> listA = flatten(typeA);
-        List<NamedType> listB = flatten(typeB);
-        if (listA.size() != listB.size()) {
-            throw new RuntimeException("Can't match up structures: typeA has " +
-                Utils.join(listA, ", ") + ", but typeB has " +
-                Utils.join(listB, ", "));
-        }
-        for (int i = 0; i < listA.size(); i++) {
-            NamedType entryA = listA.get(i);
-            NamedType entryB = listB.get(i);
-            if (!entryA.hasSimilarType(entryB)) {
-                throw new RuntimeException("Type " + entryA + " in schema A " +
-                    "does not match type " + entryB + " in schema B.");
-            }
-            if (entryA.type.isNullable() != entryB.type.isNullable()) {
-                throw new RuntimeException(String.format(
-                    "Type %s in Schema A is %s, but type %s in " +
-                        "Schema B is %s",
-                    entryA, entryA.type.isNullable() ? "nullable" : "non-nullable",
-                    entryB, entryB.type.isNullable() ? "nullable" : "non-nullable"));
-            }
-            if (entryA.type.isArray()) {
-                compareTypes(new NamedType(entryA.name, entryA.type.arrayElementType().get()),
-                             new NamedType(entryB.name, entryB.type.arrayElementType().get()));
-            }
-        }
-    }
-
     /**

Review comment:
       this method is never used.

##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -343,9 +174,15 @@ public Struct parseResponse(short version, ByteBuffer buffer) {
                 throw new IllegalStateException("Response schema for api " + name + " for version " + i + " is null");
         }
 
+        this.messageType = messageType;
+        this.id = id;
+        this.name = name;

Review comment:
       Could we fix the comment about "name" as it is used to be a part of metrics name (see https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/network/RequestChannel.scala#L239). The origin comment is stale.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] ijuma commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r543816097



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -361,69 +172,35 @@ private static boolean shouldRetainsBufferReference(Schema[] requestSchemas) {
     }
 
     public static ApiKeys forId(int id) {

Review comment:
       We used `int` here because `short` in Java is a second class citizen and casting is required in some cases. If we decide to change this, a separate PR (as you suggested) would be better.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] chia7712 commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r543985886



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -157,124 +38,78 @@
  * Identifiers for all the Kafka APIs
  */
 public enum ApiKeys {
-    PRODUCE(0, "Produce", ProduceRequestData.SCHEMAS, ProduceResponseData.SCHEMAS),
-    FETCH(1, "Fetch", FetchRequestData.SCHEMAS, FetchResponseData.SCHEMAS),
-    LIST_OFFSETS(2, "ListOffsets", ListOffsetRequestData.SCHEMAS, ListOffsetResponseData.SCHEMAS),
-    METADATA(3, "Metadata", MetadataRequestData.SCHEMAS, MetadataResponseData.SCHEMAS),
-    LEADER_AND_ISR(4, "LeaderAndIsr", true, LeaderAndIsrRequestData.SCHEMAS, LeaderAndIsrResponseData.SCHEMAS),
-    STOP_REPLICA(5, "StopReplica", true, StopReplicaRequestData.SCHEMAS, StopReplicaResponseData.SCHEMAS),
-    UPDATE_METADATA(6, "UpdateMetadata", true, UpdateMetadataRequestData.SCHEMAS, UpdateMetadataResponseData.SCHEMAS),
-    CONTROLLED_SHUTDOWN(7, "ControlledShutdown", true, ControlledShutdownRequestData.SCHEMAS,
-            ControlledShutdownResponseData.SCHEMAS),
-    OFFSET_COMMIT(8, "OffsetCommit", OffsetCommitRequestData.SCHEMAS, OffsetCommitResponseData.SCHEMAS),
-    OFFSET_FETCH(9, "OffsetFetch", OffsetFetchRequestData.SCHEMAS, OffsetFetchResponseData.SCHEMAS),
-    FIND_COORDINATOR(10, "FindCoordinator", FindCoordinatorRequestData.SCHEMAS,
-            FindCoordinatorResponseData.SCHEMAS),
-    JOIN_GROUP(11, "JoinGroup", JoinGroupRequestData.SCHEMAS, JoinGroupResponseData.SCHEMAS),
-    HEARTBEAT(12, "Heartbeat", HeartbeatRequestData.SCHEMAS, HeartbeatResponseData.SCHEMAS),
-    LEAVE_GROUP(13, "LeaveGroup", LeaveGroupRequestData.SCHEMAS, LeaveGroupResponseData.SCHEMAS),
-    SYNC_GROUP(14, "SyncGroup", SyncGroupRequestData.SCHEMAS, SyncGroupResponseData.SCHEMAS),
-    DESCRIBE_GROUPS(15, "DescribeGroups", DescribeGroupsRequestData.SCHEMAS,
-            DescribeGroupsResponseData.SCHEMAS),
-    LIST_GROUPS(16, "ListGroups", ListGroupsRequestData.SCHEMAS, ListGroupsResponseData.SCHEMAS),
-    SASL_HANDSHAKE(17, "SaslHandshake", SaslHandshakeRequestData.SCHEMAS, SaslHandshakeResponseData.SCHEMAS),
-    API_VERSIONS(18, "ApiVersions", ApiVersionsRequestData.SCHEMAS, ApiVersionsResponseData.SCHEMAS) {
-        @Override
-        public Struct parseResponse(short version, ByteBuffer buffer) {
-            // Fallback to version 0 for ApiVersions response. If a client sends an ApiVersionsRequest
-            // using a version higher than that supported by the broker, a version 0 response is sent
-            // to the client indicating UNSUPPORTED_VERSION.
-            return parseResponse(version, buffer, (short) 0);
-        }
-    },
-    CREATE_TOPICS(19, "CreateTopics", CreateTopicsRequestData.SCHEMAS, CreateTopicsResponseData.SCHEMAS, true),
-    DELETE_TOPICS(20, "DeleteTopics", DeleteTopicsRequestData.SCHEMAS, DeleteTopicsResponseData.SCHEMAS, true),
-    DELETE_RECORDS(21, "DeleteRecords", DeleteRecordsRequestData.SCHEMAS, DeleteRecordsResponseData.SCHEMAS),
-    INIT_PRODUCER_ID(22, "InitProducerId", InitProducerIdRequestData.SCHEMAS, InitProducerIdResponseData.SCHEMAS),
-    OFFSET_FOR_LEADER_EPOCH(23, "OffsetForLeaderEpoch", false, OffsetForLeaderEpochRequestData.SCHEMAS,
-        OffsetForLeaderEpochResponseData.SCHEMAS),
-    ADD_PARTITIONS_TO_TXN(24, "AddPartitionsToTxn", false, RecordBatch.MAGIC_VALUE_V2,
-            AddPartitionsToTxnRequestData.SCHEMAS, AddPartitionsToTxnResponseData.SCHEMAS),
-    ADD_OFFSETS_TO_TXN(25, "AddOffsetsToTxn", false, RecordBatch.MAGIC_VALUE_V2, AddOffsetsToTxnRequestData.SCHEMAS,
-            AddOffsetsToTxnResponseData.SCHEMAS),
-    END_TXN(26, "EndTxn", false, RecordBatch.MAGIC_VALUE_V2, EndTxnRequestData.SCHEMAS, EndTxnResponseData.SCHEMAS),
-    WRITE_TXN_MARKERS(27, "WriteTxnMarkers", true, RecordBatch.MAGIC_VALUE_V2, WriteTxnMarkersRequestData.SCHEMAS,
-            WriteTxnMarkersResponseData.SCHEMAS),
-    TXN_OFFSET_COMMIT(28, "TxnOffsetCommit", false, RecordBatch.MAGIC_VALUE_V2, TxnOffsetCommitRequestData.SCHEMAS,
-            TxnOffsetCommitResponseData.SCHEMAS),
-    DESCRIBE_ACLS(29, "DescribeAcls", DescribeAclsRequestData.SCHEMAS, DescribeAclsResponseData.SCHEMAS),
-    CREATE_ACLS(30, "CreateAcls", CreateAclsRequestData.SCHEMAS, CreateAclsResponseData.SCHEMAS, true),
-    DELETE_ACLS(31, "DeleteAcls", DeleteAclsRequestData.SCHEMAS, DeleteAclsResponseData.SCHEMAS, true),
-    DESCRIBE_CONFIGS(32, "DescribeConfigs", DescribeConfigsRequestData.SCHEMAS,
-             DescribeConfigsResponseData.SCHEMAS),
-    ALTER_CONFIGS(33, "AlterConfigs", AlterConfigsRequestData.SCHEMAS,
-            AlterConfigsResponseData.SCHEMAS, true),
-    ALTER_REPLICA_LOG_DIRS(34, "AlterReplicaLogDirs", AlterReplicaLogDirsRequestData.SCHEMAS,
-            AlterReplicaLogDirsResponseData.SCHEMAS),
-    DESCRIBE_LOG_DIRS(35, "DescribeLogDirs", DescribeLogDirsRequestData.SCHEMAS,
-            DescribeLogDirsResponseData.SCHEMAS),
-    SASL_AUTHENTICATE(36, "SaslAuthenticate", SaslAuthenticateRequestData.SCHEMAS,
-            SaslAuthenticateResponseData.SCHEMAS),
-    CREATE_PARTITIONS(37, "CreatePartitions", CreatePartitionsRequestData.SCHEMAS,
-            CreatePartitionsResponseData.SCHEMAS, true),
-    CREATE_DELEGATION_TOKEN(38, "CreateDelegationToken", CreateDelegationTokenRequestData.SCHEMAS,
-            CreateDelegationTokenResponseData.SCHEMAS, true),
-    RENEW_DELEGATION_TOKEN(39, "RenewDelegationToken", RenewDelegationTokenRequestData.SCHEMAS,
-            RenewDelegationTokenResponseData.SCHEMAS, true),
-    EXPIRE_DELEGATION_TOKEN(40, "ExpireDelegationToken", ExpireDelegationTokenRequestData.SCHEMAS,
-            ExpireDelegationTokenResponseData.SCHEMAS, true),
-    DESCRIBE_DELEGATION_TOKEN(41, "DescribeDelegationToken", DescribeDelegationTokenRequestData.SCHEMAS,
-            DescribeDelegationTokenResponseData.SCHEMAS),
-    DELETE_GROUPS(42, "DeleteGroups", DeleteGroupsRequestData.SCHEMAS, DeleteGroupsResponseData.SCHEMAS),
-    ELECT_LEADERS(43, "ElectLeaders", ElectLeadersRequestData.SCHEMAS,
-            ElectLeadersResponseData.SCHEMAS),
-    INCREMENTAL_ALTER_CONFIGS(44, "IncrementalAlterConfigs", IncrementalAlterConfigsRequestData.SCHEMAS,
-            IncrementalAlterConfigsResponseData.SCHEMAS, true),
-    ALTER_PARTITION_REASSIGNMENTS(45, "AlterPartitionReassignments", AlterPartitionReassignmentsRequestData.SCHEMAS,
-            AlterPartitionReassignmentsResponseData.SCHEMAS, true),
-    LIST_PARTITION_REASSIGNMENTS(46, "ListPartitionReassignments", ListPartitionReassignmentsRequestData.SCHEMAS,
-            ListPartitionReassignmentsResponseData.SCHEMAS),
-    OFFSET_DELETE(47, "OffsetDelete", OffsetDeleteRequestData.SCHEMAS, OffsetDeleteResponseData.SCHEMAS),
-    DESCRIBE_CLIENT_QUOTAS(48, "DescribeClientQuotas", DescribeClientQuotasRequestData.SCHEMAS,
-            DescribeClientQuotasResponseData.SCHEMAS),
-    ALTER_CLIENT_QUOTAS(49, "AlterClientQuotas", AlterClientQuotasRequestData.SCHEMAS,
-            AlterClientQuotasResponseData.SCHEMAS, true),
-    DESCRIBE_USER_SCRAM_CREDENTIALS(50, "DescribeUserScramCredentials", DescribeUserScramCredentialsRequestData.SCHEMAS,
-            DescribeUserScramCredentialsResponseData.SCHEMAS),
-    ALTER_USER_SCRAM_CREDENTIALS(51, "AlterUserScramCredentials", AlterUserScramCredentialsRequestData.SCHEMAS,
-            AlterUserScramCredentialsResponseData.SCHEMAS, true),
-    VOTE(52, "Vote", true, false,
-        VoteRequestData.SCHEMAS, VoteResponseData.SCHEMAS),
-    BEGIN_QUORUM_EPOCH(53, "BeginQuorumEpoch", true, false,
-        BeginQuorumEpochRequestData.SCHEMAS, BeginQuorumEpochResponseData.SCHEMAS),
-    END_QUORUM_EPOCH(54, "EndQuorumEpoch", true, false,
-        EndQuorumEpochRequestData.SCHEMAS, EndQuorumEpochResponseData.SCHEMAS),
-    DESCRIBE_QUORUM(55, "DescribeQuorum", true, false,
-        DescribeQuorumRequestData.SCHEMAS, DescribeQuorumResponseData.SCHEMAS),
-    ALTER_ISR(56, "AlterIsr", true, AlterIsrRequestData.SCHEMAS, AlterIsrResponseData.SCHEMAS),
-    UPDATE_FEATURES(57, "UpdateFeatures",
-        UpdateFeaturesRequestData.SCHEMAS, UpdateFeaturesResponseData.SCHEMAS, true),
-    ENVELOPE(58, "Envelope", true, false, EnvelopeRequestData.SCHEMAS, EnvelopeResponseData.SCHEMAS);
-
-    private static final ApiKeys[] ID_TO_TYPE;
-    private static final int MIN_API_KEY = 0;
-    public static final int MAX_API_KEY;
-
+    PRODUCE(ApiMessageType.PRODUCE),
+    FETCH(ApiMessageType.FETCH),
+    LIST_OFFSETS(ApiMessageType.LIST_OFFSETS),
+    METADATA(ApiMessageType.METADATA),
+    LEADER_AND_ISR(ApiMessageType.LEADER_AND_ISR, true),
+    STOP_REPLICA(ApiMessageType.STOP_REPLICA, true),
+    UPDATE_METADATA(ApiMessageType.UPDATE_METADATA, true),
+    CONTROLLED_SHUTDOWN(ApiMessageType.CONTROLLED_SHUTDOWN, true),
+    OFFSET_COMMIT(ApiMessageType.OFFSET_COMMIT),
+    OFFSET_FETCH(ApiMessageType.OFFSET_FETCH),
+    FIND_COORDINATOR(ApiMessageType.FIND_COORDINATOR),
+    JOIN_GROUP(ApiMessageType.JOIN_GROUP),
+    HEARTBEAT(ApiMessageType.HEARTBEAT),
+    LEAVE_GROUP(ApiMessageType.LEAVE_GROUP),
+    SYNC_GROUP(ApiMessageType.SYNC_GROUP),
+    DESCRIBE_GROUPS(ApiMessageType.DESCRIBE_GROUPS),
+    LIST_GROUPS(ApiMessageType.LIST_GROUPS),
+    SASL_HANDSHAKE(ApiMessageType.SASL_HANDSHAKE),
+    API_VERSIONS(ApiMessageType.API_VERSIONS),
+    CREATE_TOPICS(ApiMessageType.CREATE_TOPICS, false, true),
+    DELETE_TOPICS(ApiMessageType.DELETE_TOPICS, false, true),
+    DELETE_RECORDS(ApiMessageType.DELETE_RECORDS),
+    INIT_PRODUCER_ID(ApiMessageType.INIT_PRODUCER_ID),
+    OFFSET_FOR_LEADER_EPOCH(ApiMessageType.OFFSET_FOR_LEADER_EPOCH),
+    ADD_PARTITIONS_TO_TXN(ApiMessageType.ADD_PARTITIONS_TO_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    ADD_OFFSETS_TO_TXN(ApiMessageType.ADD_OFFSETS_TO_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    END_TXN(ApiMessageType.END_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    WRITE_TXN_MARKERS(ApiMessageType.WRITE_TXN_MARKERS, true, RecordBatch.MAGIC_VALUE_V2, false),
+    TXN_OFFSET_COMMIT(ApiMessageType.TXN_OFFSET_COMMIT, false, RecordBatch.MAGIC_VALUE_V2, false),
+    DESCRIBE_ACLS(ApiMessageType.DESCRIBE_ACLS),
+    CREATE_ACLS(ApiMessageType.CREATE_ACLS, false, true),
+    DELETE_ACLS(ApiMessageType.DELETE_ACLS, false, true),
+    DESCRIBE_CONFIGS(ApiMessageType.DESCRIBE_CONFIGS),
+    ALTER_CONFIGS(ApiMessageType.ALTER_CONFIGS, false, true),
+    ALTER_REPLICA_LOG_DIRS(ApiMessageType.ALTER_REPLICA_LOG_DIRS),
+    DESCRIBE_LOG_DIRS(ApiMessageType.DESCRIBE_LOG_DIRS),
+    SASL_AUTHENTICATE(ApiMessageType.SASL_AUTHENTICATE),
+    CREATE_PARTITIONS(ApiMessageType.CREATE_PARTITIONS, false, true),
+    CREATE_DELEGATION_TOKEN(ApiMessageType.CREATE_DELEGATION_TOKEN, false, true),
+    RENEW_DELEGATION_TOKEN(ApiMessageType.RENEW_DELEGATION_TOKEN, false, true),
+    EXPIRE_DELEGATION_TOKEN(ApiMessageType.EXPIRE_DELEGATION_TOKEN, false, true),
+    DESCRIBE_DELEGATION_TOKEN(ApiMessageType.DESCRIBE_DELEGATION_TOKEN),
+    DELETE_GROUPS(ApiMessageType.DELETE_GROUPS),
+    ELECT_LEADERS(ApiMessageType.ELECT_LEADERS),
+    INCREMENTAL_ALTER_CONFIGS(ApiMessageType.INCREMENTAL_ALTER_CONFIGS, false, true),
+    ALTER_PARTITION_REASSIGNMENTS(ApiMessageType.ALTER_PARTITION_REASSIGNMENTS, false, true),
+    LIST_PARTITION_REASSIGNMENTS(ApiMessageType.LIST_PARTITION_REASSIGNMENTS),
+    OFFSET_DELETE(ApiMessageType.OFFSET_DELETE),
+    DESCRIBE_CLIENT_QUOTAS(ApiMessageType.DESCRIBE_CLIENT_QUOTAS),
+    ALTER_CLIENT_QUOTAS(ApiMessageType.ALTER_CLIENT_QUOTAS, false, true),
+    DESCRIBE_USER_SCRAM_CREDENTIALS(ApiMessageType.DESCRIBE_USER_SCRAM_CREDENTIALS),
+    ALTER_USER_SCRAM_CREDENTIALS(ApiMessageType.ALTER_USER_SCRAM_CREDENTIALS, false, true),
+    VOTE(ApiMessageType.VOTE, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    BEGIN_QUORUM_EPOCH(ApiMessageType.BEGIN_QUORUM_EPOCH, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    END_QUORUM_EPOCH(ApiMessageType.END_QUORUM_EPOCH, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    DESCRIBE_QUORUM(ApiMessageType.DESCRIBE_QUORUM, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    ALTER_ISR(ApiMessageType.ALTER_ISR, true),
+    UPDATE_FEATURES(ApiMessageType.UPDATE_FEATURES, false, true),
+    ENVELOPE(ApiMessageType.ENVELOPE, true, RecordBatch.MAGIC_VALUE_V0, false, false);
+
+    private static final Map<Integer, ApiKeys> ID_TO_TYPE = new HashMap<>();

Review comment:
       It seems to me this initialization can be rewrite by following code.
   ```
       private static final Map<Integer, ApiKeys> ID_TO_TYPE = Arrays.stream(ApiKeys.values())
               .collect(Collectors.toMap(key -> (int) key.id, Function.identity()));
   ```
   
   The benefit is that he static block can be removed.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] ijuma commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType and removing unused methods

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r542987065



##########
File path: generator/src/main/java/org/apache/kafka/message/ApiMessageTypeGenerator.java
##########
@@ -169,7 +169,7 @@ private void generateEnumValues() {
     }
 
     private void generateInstanceVariables() {
-        buffer.printf("private final String name;%n");
+        buffer.printf("public final String name;%n");

Review comment:
       Yeah, that's an existing problem with ApiKeys too. The enum method cannot be overridden. I think it should be tackled separately.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] ijuma commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r544012051



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -157,124 +38,78 @@
  * Identifiers for all the Kafka APIs
  */
 public enum ApiKeys {
-    PRODUCE(0, "Produce", ProduceRequestData.SCHEMAS, ProduceResponseData.SCHEMAS),
-    FETCH(1, "Fetch", FetchRequestData.SCHEMAS, FetchResponseData.SCHEMAS),
-    LIST_OFFSETS(2, "ListOffsets", ListOffsetRequestData.SCHEMAS, ListOffsetResponseData.SCHEMAS),
-    METADATA(3, "Metadata", MetadataRequestData.SCHEMAS, MetadataResponseData.SCHEMAS),
-    LEADER_AND_ISR(4, "LeaderAndIsr", true, LeaderAndIsrRequestData.SCHEMAS, LeaderAndIsrResponseData.SCHEMAS),
-    STOP_REPLICA(5, "StopReplica", true, StopReplicaRequestData.SCHEMAS, StopReplicaResponseData.SCHEMAS),
-    UPDATE_METADATA(6, "UpdateMetadata", true, UpdateMetadataRequestData.SCHEMAS, UpdateMetadataResponseData.SCHEMAS),
-    CONTROLLED_SHUTDOWN(7, "ControlledShutdown", true, ControlledShutdownRequestData.SCHEMAS,
-            ControlledShutdownResponseData.SCHEMAS),
-    OFFSET_COMMIT(8, "OffsetCommit", OffsetCommitRequestData.SCHEMAS, OffsetCommitResponseData.SCHEMAS),
-    OFFSET_FETCH(9, "OffsetFetch", OffsetFetchRequestData.SCHEMAS, OffsetFetchResponseData.SCHEMAS),
-    FIND_COORDINATOR(10, "FindCoordinator", FindCoordinatorRequestData.SCHEMAS,
-            FindCoordinatorResponseData.SCHEMAS),
-    JOIN_GROUP(11, "JoinGroup", JoinGroupRequestData.SCHEMAS, JoinGroupResponseData.SCHEMAS),
-    HEARTBEAT(12, "Heartbeat", HeartbeatRequestData.SCHEMAS, HeartbeatResponseData.SCHEMAS),
-    LEAVE_GROUP(13, "LeaveGroup", LeaveGroupRequestData.SCHEMAS, LeaveGroupResponseData.SCHEMAS),
-    SYNC_GROUP(14, "SyncGroup", SyncGroupRequestData.SCHEMAS, SyncGroupResponseData.SCHEMAS),
-    DESCRIBE_GROUPS(15, "DescribeGroups", DescribeGroupsRequestData.SCHEMAS,
-            DescribeGroupsResponseData.SCHEMAS),
-    LIST_GROUPS(16, "ListGroups", ListGroupsRequestData.SCHEMAS, ListGroupsResponseData.SCHEMAS),
-    SASL_HANDSHAKE(17, "SaslHandshake", SaslHandshakeRequestData.SCHEMAS, SaslHandshakeResponseData.SCHEMAS),
-    API_VERSIONS(18, "ApiVersions", ApiVersionsRequestData.SCHEMAS, ApiVersionsResponseData.SCHEMAS) {
-        @Override
-        public Struct parseResponse(short version, ByteBuffer buffer) {
-            // Fallback to version 0 for ApiVersions response. If a client sends an ApiVersionsRequest
-            // using a version higher than that supported by the broker, a version 0 response is sent
-            // to the client indicating UNSUPPORTED_VERSION.
-            return parseResponse(version, buffer, (short) 0);
-        }
-    },
-    CREATE_TOPICS(19, "CreateTopics", CreateTopicsRequestData.SCHEMAS, CreateTopicsResponseData.SCHEMAS, true),
-    DELETE_TOPICS(20, "DeleteTopics", DeleteTopicsRequestData.SCHEMAS, DeleteTopicsResponseData.SCHEMAS, true),
-    DELETE_RECORDS(21, "DeleteRecords", DeleteRecordsRequestData.SCHEMAS, DeleteRecordsResponseData.SCHEMAS),
-    INIT_PRODUCER_ID(22, "InitProducerId", InitProducerIdRequestData.SCHEMAS, InitProducerIdResponseData.SCHEMAS),
-    OFFSET_FOR_LEADER_EPOCH(23, "OffsetForLeaderEpoch", false, OffsetForLeaderEpochRequestData.SCHEMAS,
-        OffsetForLeaderEpochResponseData.SCHEMAS),
-    ADD_PARTITIONS_TO_TXN(24, "AddPartitionsToTxn", false, RecordBatch.MAGIC_VALUE_V2,
-            AddPartitionsToTxnRequestData.SCHEMAS, AddPartitionsToTxnResponseData.SCHEMAS),
-    ADD_OFFSETS_TO_TXN(25, "AddOffsetsToTxn", false, RecordBatch.MAGIC_VALUE_V2, AddOffsetsToTxnRequestData.SCHEMAS,
-            AddOffsetsToTxnResponseData.SCHEMAS),
-    END_TXN(26, "EndTxn", false, RecordBatch.MAGIC_VALUE_V2, EndTxnRequestData.SCHEMAS, EndTxnResponseData.SCHEMAS),
-    WRITE_TXN_MARKERS(27, "WriteTxnMarkers", true, RecordBatch.MAGIC_VALUE_V2, WriteTxnMarkersRequestData.SCHEMAS,
-            WriteTxnMarkersResponseData.SCHEMAS),
-    TXN_OFFSET_COMMIT(28, "TxnOffsetCommit", false, RecordBatch.MAGIC_VALUE_V2, TxnOffsetCommitRequestData.SCHEMAS,
-            TxnOffsetCommitResponseData.SCHEMAS),
-    DESCRIBE_ACLS(29, "DescribeAcls", DescribeAclsRequestData.SCHEMAS, DescribeAclsResponseData.SCHEMAS),
-    CREATE_ACLS(30, "CreateAcls", CreateAclsRequestData.SCHEMAS, CreateAclsResponseData.SCHEMAS, true),
-    DELETE_ACLS(31, "DeleteAcls", DeleteAclsRequestData.SCHEMAS, DeleteAclsResponseData.SCHEMAS, true),
-    DESCRIBE_CONFIGS(32, "DescribeConfigs", DescribeConfigsRequestData.SCHEMAS,
-             DescribeConfigsResponseData.SCHEMAS),
-    ALTER_CONFIGS(33, "AlterConfigs", AlterConfigsRequestData.SCHEMAS,
-            AlterConfigsResponseData.SCHEMAS, true),
-    ALTER_REPLICA_LOG_DIRS(34, "AlterReplicaLogDirs", AlterReplicaLogDirsRequestData.SCHEMAS,
-            AlterReplicaLogDirsResponseData.SCHEMAS),
-    DESCRIBE_LOG_DIRS(35, "DescribeLogDirs", DescribeLogDirsRequestData.SCHEMAS,
-            DescribeLogDirsResponseData.SCHEMAS),
-    SASL_AUTHENTICATE(36, "SaslAuthenticate", SaslAuthenticateRequestData.SCHEMAS,
-            SaslAuthenticateResponseData.SCHEMAS),
-    CREATE_PARTITIONS(37, "CreatePartitions", CreatePartitionsRequestData.SCHEMAS,
-            CreatePartitionsResponseData.SCHEMAS, true),
-    CREATE_DELEGATION_TOKEN(38, "CreateDelegationToken", CreateDelegationTokenRequestData.SCHEMAS,
-            CreateDelegationTokenResponseData.SCHEMAS, true),
-    RENEW_DELEGATION_TOKEN(39, "RenewDelegationToken", RenewDelegationTokenRequestData.SCHEMAS,
-            RenewDelegationTokenResponseData.SCHEMAS, true),
-    EXPIRE_DELEGATION_TOKEN(40, "ExpireDelegationToken", ExpireDelegationTokenRequestData.SCHEMAS,
-            ExpireDelegationTokenResponseData.SCHEMAS, true),
-    DESCRIBE_DELEGATION_TOKEN(41, "DescribeDelegationToken", DescribeDelegationTokenRequestData.SCHEMAS,
-            DescribeDelegationTokenResponseData.SCHEMAS),
-    DELETE_GROUPS(42, "DeleteGroups", DeleteGroupsRequestData.SCHEMAS, DeleteGroupsResponseData.SCHEMAS),
-    ELECT_LEADERS(43, "ElectLeaders", ElectLeadersRequestData.SCHEMAS,
-            ElectLeadersResponseData.SCHEMAS),
-    INCREMENTAL_ALTER_CONFIGS(44, "IncrementalAlterConfigs", IncrementalAlterConfigsRequestData.SCHEMAS,
-            IncrementalAlterConfigsResponseData.SCHEMAS, true),
-    ALTER_PARTITION_REASSIGNMENTS(45, "AlterPartitionReassignments", AlterPartitionReassignmentsRequestData.SCHEMAS,
-            AlterPartitionReassignmentsResponseData.SCHEMAS, true),
-    LIST_PARTITION_REASSIGNMENTS(46, "ListPartitionReassignments", ListPartitionReassignmentsRequestData.SCHEMAS,
-            ListPartitionReassignmentsResponseData.SCHEMAS),
-    OFFSET_DELETE(47, "OffsetDelete", OffsetDeleteRequestData.SCHEMAS, OffsetDeleteResponseData.SCHEMAS),
-    DESCRIBE_CLIENT_QUOTAS(48, "DescribeClientQuotas", DescribeClientQuotasRequestData.SCHEMAS,
-            DescribeClientQuotasResponseData.SCHEMAS),
-    ALTER_CLIENT_QUOTAS(49, "AlterClientQuotas", AlterClientQuotasRequestData.SCHEMAS,
-            AlterClientQuotasResponseData.SCHEMAS, true),
-    DESCRIBE_USER_SCRAM_CREDENTIALS(50, "DescribeUserScramCredentials", DescribeUserScramCredentialsRequestData.SCHEMAS,
-            DescribeUserScramCredentialsResponseData.SCHEMAS),
-    ALTER_USER_SCRAM_CREDENTIALS(51, "AlterUserScramCredentials", AlterUserScramCredentialsRequestData.SCHEMAS,
-            AlterUserScramCredentialsResponseData.SCHEMAS, true),
-    VOTE(52, "Vote", true, false,
-        VoteRequestData.SCHEMAS, VoteResponseData.SCHEMAS),
-    BEGIN_QUORUM_EPOCH(53, "BeginQuorumEpoch", true, false,
-        BeginQuorumEpochRequestData.SCHEMAS, BeginQuorumEpochResponseData.SCHEMAS),
-    END_QUORUM_EPOCH(54, "EndQuorumEpoch", true, false,
-        EndQuorumEpochRequestData.SCHEMAS, EndQuorumEpochResponseData.SCHEMAS),
-    DESCRIBE_QUORUM(55, "DescribeQuorum", true, false,
-        DescribeQuorumRequestData.SCHEMAS, DescribeQuorumResponseData.SCHEMAS),
-    ALTER_ISR(56, "AlterIsr", true, AlterIsrRequestData.SCHEMAS, AlterIsrResponseData.SCHEMAS),
-    UPDATE_FEATURES(57, "UpdateFeatures",
-        UpdateFeaturesRequestData.SCHEMAS, UpdateFeaturesResponseData.SCHEMAS, true),
-    ENVELOPE(58, "Envelope", true, false, EnvelopeRequestData.SCHEMAS, EnvelopeResponseData.SCHEMAS);
-
-    private static final ApiKeys[] ID_TO_TYPE;
-    private static final int MIN_API_KEY = 0;
-    public static final int MAX_API_KEY;
-
+    PRODUCE(ApiMessageType.PRODUCE),
+    FETCH(ApiMessageType.FETCH),
+    LIST_OFFSETS(ApiMessageType.LIST_OFFSETS),
+    METADATA(ApiMessageType.METADATA),
+    LEADER_AND_ISR(ApiMessageType.LEADER_AND_ISR, true),
+    STOP_REPLICA(ApiMessageType.STOP_REPLICA, true),
+    UPDATE_METADATA(ApiMessageType.UPDATE_METADATA, true),
+    CONTROLLED_SHUTDOWN(ApiMessageType.CONTROLLED_SHUTDOWN, true),
+    OFFSET_COMMIT(ApiMessageType.OFFSET_COMMIT),
+    OFFSET_FETCH(ApiMessageType.OFFSET_FETCH),
+    FIND_COORDINATOR(ApiMessageType.FIND_COORDINATOR),
+    JOIN_GROUP(ApiMessageType.JOIN_GROUP),
+    HEARTBEAT(ApiMessageType.HEARTBEAT),
+    LEAVE_GROUP(ApiMessageType.LEAVE_GROUP),
+    SYNC_GROUP(ApiMessageType.SYNC_GROUP),
+    DESCRIBE_GROUPS(ApiMessageType.DESCRIBE_GROUPS),
+    LIST_GROUPS(ApiMessageType.LIST_GROUPS),
+    SASL_HANDSHAKE(ApiMessageType.SASL_HANDSHAKE),
+    API_VERSIONS(ApiMessageType.API_VERSIONS),
+    CREATE_TOPICS(ApiMessageType.CREATE_TOPICS, false, true),
+    DELETE_TOPICS(ApiMessageType.DELETE_TOPICS, false, true),
+    DELETE_RECORDS(ApiMessageType.DELETE_RECORDS),
+    INIT_PRODUCER_ID(ApiMessageType.INIT_PRODUCER_ID),
+    OFFSET_FOR_LEADER_EPOCH(ApiMessageType.OFFSET_FOR_LEADER_EPOCH),
+    ADD_PARTITIONS_TO_TXN(ApiMessageType.ADD_PARTITIONS_TO_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    ADD_OFFSETS_TO_TXN(ApiMessageType.ADD_OFFSETS_TO_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    END_TXN(ApiMessageType.END_TXN, false, RecordBatch.MAGIC_VALUE_V2, false),
+    WRITE_TXN_MARKERS(ApiMessageType.WRITE_TXN_MARKERS, true, RecordBatch.MAGIC_VALUE_V2, false),
+    TXN_OFFSET_COMMIT(ApiMessageType.TXN_OFFSET_COMMIT, false, RecordBatch.MAGIC_VALUE_V2, false),
+    DESCRIBE_ACLS(ApiMessageType.DESCRIBE_ACLS),
+    CREATE_ACLS(ApiMessageType.CREATE_ACLS, false, true),
+    DELETE_ACLS(ApiMessageType.DELETE_ACLS, false, true),
+    DESCRIBE_CONFIGS(ApiMessageType.DESCRIBE_CONFIGS),
+    ALTER_CONFIGS(ApiMessageType.ALTER_CONFIGS, false, true),
+    ALTER_REPLICA_LOG_DIRS(ApiMessageType.ALTER_REPLICA_LOG_DIRS),
+    DESCRIBE_LOG_DIRS(ApiMessageType.DESCRIBE_LOG_DIRS),
+    SASL_AUTHENTICATE(ApiMessageType.SASL_AUTHENTICATE),
+    CREATE_PARTITIONS(ApiMessageType.CREATE_PARTITIONS, false, true),
+    CREATE_DELEGATION_TOKEN(ApiMessageType.CREATE_DELEGATION_TOKEN, false, true),
+    RENEW_DELEGATION_TOKEN(ApiMessageType.RENEW_DELEGATION_TOKEN, false, true),
+    EXPIRE_DELEGATION_TOKEN(ApiMessageType.EXPIRE_DELEGATION_TOKEN, false, true),
+    DESCRIBE_DELEGATION_TOKEN(ApiMessageType.DESCRIBE_DELEGATION_TOKEN),
+    DELETE_GROUPS(ApiMessageType.DELETE_GROUPS),
+    ELECT_LEADERS(ApiMessageType.ELECT_LEADERS),
+    INCREMENTAL_ALTER_CONFIGS(ApiMessageType.INCREMENTAL_ALTER_CONFIGS, false, true),
+    ALTER_PARTITION_REASSIGNMENTS(ApiMessageType.ALTER_PARTITION_REASSIGNMENTS, false, true),
+    LIST_PARTITION_REASSIGNMENTS(ApiMessageType.LIST_PARTITION_REASSIGNMENTS),
+    OFFSET_DELETE(ApiMessageType.OFFSET_DELETE),
+    DESCRIBE_CLIENT_QUOTAS(ApiMessageType.DESCRIBE_CLIENT_QUOTAS),
+    ALTER_CLIENT_QUOTAS(ApiMessageType.ALTER_CLIENT_QUOTAS, false, true),
+    DESCRIBE_USER_SCRAM_CREDENTIALS(ApiMessageType.DESCRIBE_USER_SCRAM_CREDENTIALS),
+    ALTER_USER_SCRAM_CREDENTIALS(ApiMessageType.ALTER_USER_SCRAM_CREDENTIALS, false, true),
+    VOTE(ApiMessageType.VOTE, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    BEGIN_QUORUM_EPOCH(ApiMessageType.BEGIN_QUORUM_EPOCH, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    END_QUORUM_EPOCH(ApiMessageType.END_QUORUM_EPOCH, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    DESCRIBE_QUORUM(ApiMessageType.DESCRIBE_QUORUM, true, RecordBatch.MAGIC_VALUE_V0, false, false),
+    ALTER_ISR(ApiMessageType.ALTER_ISR, true),
+    UPDATE_FEATURES(ApiMessageType.UPDATE_FEATURES, false, true),
+    ENVELOPE(ApiMessageType.ENVELOPE, true, RecordBatch.MAGIC_VALUE_V0, false, false);
+
+    private static final Map<Integer, ApiKeys> ID_TO_TYPE = new HashMap<>();

Review comment:
       Updated.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [kafka] ijuma commented on a change in pull request #9748: MINOR: Simplify ApiKeys by relying on ApiMessageType and removing unused methods

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9748:
URL: https://github.com/apache/kafka/pull/9748#discussion_r543407334



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -343,9 +174,15 @@ public Struct parseResponse(short version, ByteBuffer buffer) {
                 throw new IllegalStateException("Response schema for api " + name + " for version " + i + " is null");
         }
 
+        this.messageType = messageType;
+        this.id = id;
+        this.name = name;

Review comment:
       Good catch.




----------------------------------------------------------------
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:
users@infra.apache.org