AndrewJSchofield commented on code in PR #17681:
URL: https://github.com/apache/kafka/pull/17681#discussion_r1871372474
##########
clients/src/main/resources/common/message/ApiVersionsResponse.json:
##########
@@ -64,16 +64,16 @@
"versions": "3+", "tag": 2, "taggedVersions": "3+",
"about": "List of cluster-wide finalized features. The information is
valid only if FinalizedFeaturesEpoch >= 0.",
"fields": [
Review Comment:
nit: Extra space.
##########
clients/src/main/resources/common/message/ApiVersionsResponse.json:
##########
@@ -64,16 +64,16 @@
"versions": "3+", "tag": 2, "taggedVersions": "3+",
"about": "List of cluster-wide finalized features. The information is
valid only if FinalizedFeaturesEpoch >= 0.",
"fields": [
- {"name": "Name", "type": "string", "versions": "3+", "mapKey": true,
+ { "name": "Name", "type": "string", "versions": "3+", "mapKey": true,
"about": "The name of the feature."},
- {"name": "MaxVersionLevel", "type": "int16", "versions": "3+",
+ { "name": "MaxVersionLevel", "type": "int16", "versions": "3+",
"about": "The cluster-wide finalized max version level for the
feature."},
- {"name": "MinVersionLevel", "type": "int16", "versions": "3+",
+ { "name": "MinVersionLevel", "type": "int16", "versions": "3+",
Review Comment:
nit: Extra space.
##########
clients/src/main/resources/common/message/AssignReplicasToDirsResponse.json:
##########
@@ -23,17 +23,20 @@
{ "name": "ThrottleTimeMs", "type": "int32", "versions": "0+",
"about": "The duration in milliseconds for which the request was
throttled due to a quota violation, or zero if the request did not violate any
quota." },
{ "name": "ErrorCode", "type": "int16", "versions": "0+",
- "about": "The top level response error code" },
- { "name": "Directories", "type": "[]DirectoryData", "versions": "0+",
"fields": [
- { "name": "Id", "type": "uuid", "versions": "0+", "about": "The ID of
the directory" },
- { "name": "Topics", "type": "[]TopicData", "versions": "0+", "fields": [
+ "about": "The top level response error code." },
+ { "name": "Directories", "type": "[]DirectoryData", "versions": "0+",
+ "about": "The list of directories and their assigned partitions.",
"fields": [
+ { "name": "Id", "type": "uuid", "versions": "0+", "about": "The ID of
the directory." },
Review Comment:
nit: Extra space.
##########
clients/src/main/resources/common/message/BeginQuorumEpochRequest.json:
##########
@@ -23,32 +23,33 @@
"flexibleVersions": "1+",
"fields": [
{ "name": "ClusterId", "type": "string", "versions": "0+",
- "nullableVersions": "0+", "default": "null"},
+ "nullableVersions": "0+", "default": "null",
+ "about": "The cluster id."},
Review Comment:
nit: Extra space.
##########
clients/src/main/resources/common/message/EndQuorumEpochRequest.json:
##########
@@ -24,36 +24,39 @@
"flexibleVersions": "1+",
"fields": [
{ "name": "ClusterId", "type": "string", "versions": "0+",
- "nullableVersions": "0+", "default": "null"},
- { "name": "Topics", "type": "[]TopicData",
- "versions": "0+", "fields": [
+ "nullableVersions": "0+", "default": "null",
+ "about": "The cluster id."},
+ { "name": "Topics", "type": "[]TopicData", "versions": "0+",
+ "about": "The topics to end epoch on.", "fields": [
{ "name": "TopicName", "type": "string", "versions": "0+",
"entityType": "topicName",
"about": "The topic name." },
- { "name": "Partitions", "type": "[]PartitionData",
- "versions": "0+", "fields": [
+ { "name": "Partitions", "type": "[]PartitionData", "versions": "0+",
+ "about": "The partitions to end epoch on.", "fields": [
Review Comment:
nit: I suggest just "The partitions". The "to end epoch on" is odd.
##########
clients/src/main/resources/common/message/DescribeTopicPartitionsResponse.json:
##########
@@ -59,8 +59,8 @@
{ "name": "NextCursor", "type": "Cursor", "versions": "0+",
"nullableVersions": "0+", "default": "null",
"about": "The next topic and partition index to fetch details for.",
"fields": [
{ "name": "TopicName", "type": "string", "versions": "0+",
Review Comment:
This is a bit of a mess. This line and the next both include "versions". I
suggest moving the entityType onto line 61 and just having the about
description on line 62.
##########
clients/src/main/resources/common/message/FindCoordinatorRequest.json:
##########
@@ -37,7 +37,7 @@
{ "name": "Key", "type": "string", "versions": "0-3",
"about": "The coordinator key." },
{ "name": "KeyType", "type": "int8", "versions": "1+", "default": "0",
"ignorable": false,
- "about": "The coordinator key type. (Group, transaction, etc.)" },
+ "about": "The coordinator key type. (Group, transaction, etc.)." },
Review Comment:
I think "(group, transaction, share)" are the three key types supported at
the moment.
##########
clients/src/main/resources/common/message/AssignReplicasToDirsRequest.json:
##########
@@ -22,17 +22,20 @@
"flexibleVersions": "0+",
"fields": [
{ "name": "BrokerId", "type": "int32", "versions": "0+", "entityType":
"brokerId",
- "about": "The ID of the requesting broker" },
+ "about": "The ID of the requesting broker." },
{ "name": "BrokerEpoch", "type": "int64", "versions": "0+", "default":
"-1",
- "about": "The epoch of the requesting broker" },
- { "name": "Directories", "type": "[]DirectoryData", "versions": "0+",
"fields": [
- { "name": "Id", "type": "uuid", "versions": "0+", "about": "The ID of
the directory" },
- { "name": "Topics", "type": "[]TopicData", "versions": "0+", "fields": [
+ "about": "The epoch of the requesting broker." },
+ { "name": "Directories", "type": "[]DirectoryData", "versions": "0+",
+ "about": "The directories to which replicas should be assigned.",
"fields": [
+ { "name": "Id", "type": "uuid", "versions": "0+", "about": "The ID of
the directory." },
Review Comment:
nit: Extra space.
##########
clients/src/main/resources/common/message/ApiVersionsResponse.json:
##########
@@ -64,16 +64,16 @@
"versions": "3+", "tag": 2, "taggedVersions": "3+",
"about": "List of cluster-wide finalized features. The information is
valid only if FinalizedFeaturesEpoch >= 0.",
"fields": [
- {"name": "Name", "type": "string", "versions": "3+", "mapKey": true,
+ { "name": "Name", "type": "string", "versions": "3+", "mapKey": true,
"about": "The name of the feature."},
- {"name": "MaxVersionLevel", "type": "int16", "versions": "3+",
+ { "name": "MaxVersionLevel", "type": "int16", "versions": "3+",
"about": "The cluster-wide finalized max version level for the
feature."},
- {"name": "MinVersionLevel", "type": "int16", "versions": "3+",
+ { "name": "MinVersionLevel", "type": "int16", "versions": "3+",
"about": "The cluster-wide finalized min version level for the
feature."}
]
},
{ "name": "ZkMigrationReady", "type": "bool", "versions": "3+",
"taggedVersions": "3+",
Review Comment:
nit: Extra space.
##########
clients/src/main/resources/common/message/ApiVersionsResponse.json:
##########
@@ -45,8 +45,8 @@
]},
{ "name": "ThrottleTimeMs", "type": "int32", "versions": "1+",
"ignorable": true,
"about": "The duration in milliseconds for which the request was
throttled due to a quota violation, or zero if the request did not violate any
quota." },
- { "name": "SupportedFeatures", "type": "[]SupportedFeatureKey",
"ignorable": true,
- "versions": "3+", "tag": 0, "taggedVersions": "3+",
+ { "name": "SupportedFeatures", "type": "[]SupportedFeatureKey",
"ignorable": true,
+ "versions": "3+", "tag": 0, "taggedVersions": "3+",
"about": "Features supported by the broker. Note: in v0-v3, features
with MinSupportedVersion = 0 are omitted.",
"fields": [
Review Comment:
nit: Extra space.
##########
clients/src/main/resources/common/message/BeginQuorumEpochRequest.json:
##########
@@ -23,32 +23,33 @@
"flexibleVersions": "1+",
"fields": [
{ "name": "ClusterId", "type": "string", "versions": "0+",
- "nullableVersions": "0+", "default": "null"},
+ "nullableVersions": "0+", "default": "null",
+ "about": "The cluster id."},
{ "name": "VoterId", "type": "int32", "versions": "1+", "ignorable": true,
"default": "-1", "entityType": "brokerId",
- "about": "The replica id of the voter receiving the request" },
- { "name": "Topics", "type": "[]TopicData",
- "versions": "0+", "fields": [
+ "about": "The replica id of the voter receiving the request." },
+ { "name": "Topics", "type": "[]TopicData", "versions": "0+",
+ "about": "The topics to begin the epoch on.", "fields": [
{ "name": "TopicName", "type": "string", "versions": "0+",
"entityType": "topicName",
- "about": "The topic name" },
- { "name": "Partitions", "type": "[]PartitionData",
- "versions": "0+", "fields": [
+ "about": "The topic name." },
+ { "name": "Partitions", "type": "[]PartitionData", "versions": "0+",
+ "about": "The partitions to begin the epoch on.", "fields": [
Review Comment:
As above "on" is probably better removed.
##########
clients/src/main/resources/common/message/DeleteTopicsResponse.json:
##########
@@ -39,8 +39,8 @@
{ "name": "Responses", "type": "[]DeletableTopicResult", "versions": "0+",
"about": "The results for each topic we tried to delete.", "fields": [
{ "name": "Name", "type": "string", "versions": "0+",
"nullableVersions": "6+", "mapKey": true, "entityType": "topicName",
- "about": "The topic name" },
- {"name": "TopicId", "type": "uuid", "versions": "6+", "ignorable": true,
"about": "the unique topic ID"},
+ "about": "The topic name." },
+ {"name": "TopicId", "type": "uuid", "versions": "6+", "ignorable": true,
"about": "the unique topic ID."},
Review Comment:
nit: Capitalization "The unique topic ID."
##########
clients/src/main/resources/common/message/DescribeLogDirsResponse.json:
##########
@@ -38,16 +38,16 @@
"about": "Each topic.", "fields": [
{ "name": "Name", "type": "string", "versions": "0+", "entityType":
"topicName",
"about": "The topic name." },
- { "name": "Partitions", "type": "[]DescribeLogDirsPartition",
"versions": "0+", "fields": [
+ { "name": "Partitions", "type": "[]DescribeLogDirsPartition",
"versions": "0+",
+ "about": "The describe log dirs partitions.", "fields": [
Review Comment:
Maybe "The partition information."
##########
clients/src/main/resources/common/message/BeginQuorumEpochRequest.json:
##########
@@ -23,32 +23,33 @@
"flexibleVersions": "1+",
"fields": [
{ "name": "ClusterId", "type": "string", "versions": "0+",
- "nullableVersions": "0+", "default": "null"},
+ "nullableVersions": "0+", "default": "null",
+ "about": "The cluster id."},
{ "name": "VoterId", "type": "int32", "versions": "1+", "ignorable": true,
"default": "-1", "entityType": "brokerId",
- "about": "The replica id of the voter receiving the request" },
- { "name": "Topics", "type": "[]TopicData",
- "versions": "0+", "fields": [
+ "about": "The replica id of the voter receiving the request." },
+ { "name": "Topics", "type": "[]TopicData", "versions": "0+",
+ "about": "The topics to begin the epoch on.", "fields": [
Review Comment:
KIP-595 doesn't really give the terminology to use here. To me, "to begin
the epoch on" sounds a bit strange. I suggest "The topics to begin the epoch".
##########
clients/src/main/resources/common/message/DescribeClientQuotasResponse.json:
##########
@@ -36,7 +36,7 @@
"about": "The entity name, or null if the default." }
]},
{ "name": "Values", "type": "[]ValueData", "versions": "0+",
- "about": "The quota values for the entity.", "fields": [
+ "about": "The quota values for the entity.", "fields": [
Review Comment:
Alignment does seem out on this line. Is it a tab/space problem?
##########
clients/src/main/resources/common/message/DescribeQuorumResponse.json:
##########
@@ -26,50 +26,57 @@
"about": "The top level error code."},
{ "name": "ErrorMessage", "type": "string", "versions": "2+",
"nullableVersions": "2+", "ignorable": true,
"about": "The error message, or null if there was no error." },
- { "name": "Topics", "type": "[]TopicData",
- "versions": "0+", "fields": [
+ { "name": "Topics", "type": "[]TopicData", "versions": "0+",
+ "about": "The response from the describe quorum API.", "fields": [
{ "name": "TopicName", "type": "string", "versions": "0+", "entityType":
"topicName",
"about": "The topic name." },
- { "name": "Partitions", "type": "[]PartitionData",
- "versions": "0+", "fields": [
+ { "name": "Partitions", "type": "[]PartitionData", "versions": "0+",
+ "about": "The partition data.", "fields": [
{ "name": "PartitionIndex", "type": "int32", "versions": "0+",
"about": "The partition index." },
- { "name": "ErrorCode", "type": "int16", "versions": "0+"},
+ { "name": "ErrorCode", "type": "int16", "versions": "0+",
+ "about": "The partition error code."},
{ "name": "ErrorMessage", "type": "string", "versions": "2+",
"nullableVersions": "2+", "ignorable": true,
"about": "The error message, or null if there was no error." },
{ "name": "LeaderId", "type": "int32", "versions": "0+", "entityType":
"brokerId",
"about": "The ID of the current leader or -1 if the leader is
unknown."},
{ "name": "LeaderEpoch", "type": "int32", "versions": "0+",
- "about": "The latest known leader epoch"},
- { "name": "HighWatermark", "type": "int64", "versions": "0+"},
- { "name": "CurrentVoters", "type": "[]ReplicaState", "versions": "0+"
},
- { "name": "Observers", "type": "[]ReplicaState", "versions": "0+" }
+ "about": "The latest known leader epoch."},
+ { "name": "HighWatermark", "type": "int64", "versions": "0+",
+ "about": "The high watermark of the partition."},
Review Comment:
"The high water mark" would be better. Unfortunately, it should be
"HighWaterMark" too, but that mistake is too late to fix.
##########
clients/src/main/resources/common/message/DescribeTopicPartitionsRequest.json:
##########
@@ -25,16 +25,16 @@
"about": "The topics to fetch details for.",
"fields": [
{ "name": "Name", "type": "string", "versions": "0+",
- "about": "The topic name", "versions": "0+", "entityType":
"topicName"}
+ "about": "The topic name.", "versions": "0+", "entityType":
"topicName"}
]
},
{ "name": "ResponsePartitionLimit", "type": "int32", "versions": "0+",
"default": "2000",
"about": "The maximum number of partitions included in the response." },
{ "name": "Cursor", "type": "Cursor", "versions": "0+",
"nullableVersions": "0+", "default": "null",
"about": "The first topic and partition index to fetch details for.",
"fields": [
{ "name": "TopicName", "type": "string", "versions": "0+",
Review Comment:
This is a bit of a mess. This line and the next both include "versions".
##########
clients/src/main/resources/common/message/ApiVersionsResponse.json:
##########
@@ -64,16 +64,16 @@
"versions": "3+", "tag": 2, "taggedVersions": "3+",
"about": "List of cluster-wide finalized features. The information is
valid only if FinalizedFeaturesEpoch >= 0.",
"fields": [
- {"name": "Name", "type": "string", "versions": "3+", "mapKey": true,
+ { "name": "Name", "type": "string", "versions": "3+", "mapKey": true,
"about": "The name of the feature."},
- {"name": "MaxVersionLevel", "type": "int16", "versions": "3+",
+ { "name": "MaxVersionLevel", "type": "int16", "versions": "3+",
Review Comment:
nit: Extra space.
##########
clients/src/main/resources/common/message/ListTransactionsResponse.json:
##########
@@ -23,14 +23,18 @@
"fields": [
{ "name": "ThrottleTimeMs", "type": "int32", "versions": "0+",
"about": "The duration in milliseconds for which the request was
throttled due to a quota violation, or zero if the request did not violate any
quota." },
- { "name": "ErrorCode", "type": "int16", "versions": "0+" },
+ { "name": "ErrorCode", "type": "int16", "versions": "0+",
+ "about": "The error code, or 0 if there was no error."},
{ "name": "UnknownStateFilters", "type": "[]string", "versions": "0+",
- "about": "Set of state filters provided in the request which were
unknown to the transaction coordinator" },
- { "name": "TransactionStates", "type": "[]TransactionState", "versions":
"0+", "fields": [
- { "name": "TransactionalId", "type": "string", "versions": "0+",
"entityType": "transactionalId" },
- { "name": "ProducerId", "type": "int64", "versions": "0+",
"entityType": "producerId" },
+ "about": "Set of state filters provided in the request which were
unknown to the transaction coordinator." },
+ { "name": "TransactionStates", "type": "[]TransactionState", "versions":
"0+",
+ "about": "The current state of the transaction for the transactional
id.", "fields": [
+ { "name": "TransactionalId", "type": "string", "versions": "0+",
"entityType": "transactionalId",
+ "about": "The transactional id associated with the transaction."},
+ { "name": "ProducerId", "type": "int64", "versions": "0+",
"entityType": "producerId",
+ "about": "The producer id associated with the transaction."},
Review Comment:
"The producer id" would be enough.
##########
clients/src/main/resources/common/message/ListTransactionsResponse.json:
##########
@@ -23,14 +23,18 @@
"fields": [
{ "name": "ThrottleTimeMs", "type": "int32", "versions": "0+",
"about": "The duration in milliseconds for which the request was
throttled due to a quota violation, or zero if the request did not violate any
quota." },
- { "name": "ErrorCode", "type": "int16", "versions": "0+" },
+ { "name": "ErrorCode", "type": "int16", "versions": "0+",
+ "about": "The error code, or 0 if there was no error."},
{ "name": "UnknownStateFilters", "type": "[]string", "versions": "0+",
- "about": "Set of state filters provided in the request which were
unknown to the transaction coordinator" },
- { "name": "TransactionStates", "type": "[]TransactionState", "versions":
"0+", "fields": [
- { "name": "TransactionalId", "type": "string", "versions": "0+",
"entityType": "transactionalId" },
- { "name": "ProducerId", "type": "int64", "versions": "0+",
"entityType": "producerId" },
+ "about": "Set of state filters provided in the request which were
unknown to the transaction coordinator." },
+ { "name": "TransactionStates", "type": "[]TransactionState", "versions":
"0+",
+ "about": "The current state of the transaction for the transactional
id.", "fields": [
+ { "name": "TransactionalId", "type": "string", "versions": "0+",
"entityType": "transactionalId",
+ "about": "The transactional id associated with the transaction."},
Review Comment:
"The transactional id" would be enough. And there's an extra space.
##########
clients/src/main/resources/common/message/EndQuorumEpochRequest.json:
##########
@@ -24,36 +24,39 @@
"flexibleVersions": "1+",
"fields": [
{ "name": "ClusterId", "type": "string", "versions": "0+",
- "nullableVersions": "0+", "default": "null"},
- { "name": "Topics", "type": "[]TopicData",
- "versions": "0+", "fields": [
+ "nullableVersions": "0+", "default": "null",
+ "about": "The cluster id."},
+ { "name": "Topics", "type": "[]TopicData", "versions": "0+",
+ "about": "The topics to end epoch on.", "fields": [
Review Comment:
nit: I suggest just "The topics".
##########
clients/src/main/resources/common/message/ProduceResponse.json:
##########
@@ -56,18 +56,20 @@
{ "name": "LogStartOffset", "type": "int64", "versions": "5+",
"default": "-1", "ignorable": true,
"about": "The log start offset." },
{ "name": "RecordErrors", "type": "[]BatchIndexAndErrorMessage",
"versions": "8+", "ignorable": true,
- "about": "The batch indices of records that caused the batch to be
dropped", "fields": [
+ "about": "The batch indices of records that caused the batch to be
dropped.", "fields": [
{ "name": "BatchIndex", "type": "int32", "versions": "8+",
- "about": "The batch index of the record that cause the batch to be
dropped" },
+ "about": "The batch index of the record that cause the batch to be
dropped." },
Review Comment:
"that caused"
##########
transaction-coordinator/src/main/resources/common/message/TransactionLogValue.json:
##########
@@ -23,26 +23,28 @@
"flexibleVersions": "1+",
"fields": [
{ "name": "ProducerId", "type": "int64", "versions": "0+",
- "about": "Producer id in use by the transactional id"},
+ "about": "Producer id in use by the transactional id."},
Review Comment:
I suggest using either "transactional id" or "transactional ID" in this
file, but not both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
