clolov commented on code in PR #14290:
URL: https://github.com/apache/kafka/pull/14290#discussion_r1357906933
##########
metadata/src/main/resources/common/metadata/PartitionChangeRecord.json:
##########
@@ -40,6 +41,14 @@
"versions": "0+", "nullableVersions": "0+", "taggedVersions": "0+",
"tag": 4,
"about": "null if the adding replicas didn't change; the new adding
replicas otherwise." },
{ "name": "LeaderRecoveryState", "type": "int8", "default": "-1",
"versions": "0+", "taggedVersions": "0+", "tag": 5,
- "about": "-1 if it didn't change; 0 if the leader was elected from the
ISR or recovered from an unclean election; 1 if the leader that was elected
using unclean leader election and it is still recovering." }
+ "about": "-1 if it didn't change; 0 if the leader was elected from the
ISR or recovered from an unclean election; 1 if the leader that was elected
using unclean leader election and it is still recovering." },
+ { "name": "Assignment", "type": "[]ReplicaAssignment", "default": "null",
+ "versions": "1+", "nullableVersions": "1+", "taggedVersions": "1+",
"tag": 6,
Review Comment:
The KIP doesn't mention this to be a tagged field - I am happy to update the
KIP for consistency
##########
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java:
##########
@@ -1170,6 +1174,71 @@ private ConsumerGroupDescribeResponse
createConsumerGroupDescribeResponse() {
return new ConsumerGroupDescribeResponse(data);
}
+ private AssignReplicasToDirsRequest
createAssignReplicasToDirsRequest(short version) {
+ AssignReplicasToDirsRequestData data = new
AssignReplicasToDirsRequestData()
+ .setBrokerId(1)
+ .setBrokerEpoch(123L)
+ .setDirectories(Arrays.asList(
+ new AssignReplicasToDirsRequestData.DirectoryData()
+ .setId(Uuid.randomUuid())
+ .setTopics(Arrays.asList(
+ new
AssignReplicasToDirsRequestData.TopicData()
+ .setName("foo")
+ .setPartitions(Arrays.asList(
+ new
AssignReplicasToDirsRequestData.PartitionData()
+
.setPartitionIndex(8)
+ ))
+ )),
+ new AssignReplicasToDirsRequestData.DirectoryData()
+ .setId(Uuid.randomUuid())
+ .setTopics(Arrays.asList(
+ new
AssignReplicasToDirsRequestData.TopicData()
+ .setName("bar")
+ .setPartitions(Arrays.asList(
+ new
AssignReplicasToDirsRequestData.PartitionData()
+
.setPartitionIndex(2),
+ new
AssignReplicasToDirsRequestData.PartitionData()
+
.setPartitionIndex(80)
+ ))
+ ))
+ ));
+ return new AssignReplicasToDirsRequest.Builder(data).build(version);
+ }
+
+ private AssignReplicasToDirsResponse createAssignReplicasToDirsResponse() {
+ AssignReplicasToDirsResponseData data = new
AssignReplicasToDirsResponseData()
+ .setErrorCode(Errors.NONE.code())
+ .setThrottleTimeMs(123)
+ .setDirectories(Arrays.asList(
+ new AssignReplicasToDirsResponseData.DirectoryData()
+ .setId(Uuid.randomUuid())
+ .setTopics(Arrays.asList(
+ new
AssignReplicasToDirsResponseData.TopicData()
+ .setName("foo")
+ .setPartitions(Arrays.asList(
+ new
AssignReplicasToDirsResponseData.PartitionData()
+
.setPartitionIndex(8)
+
.setErrorCode(Errors.NONE.code())
+ ))
+ )),
+ new AssignReplicasToDirsResponseData.DirectoryData()
+ .setId(Uuid.randomUuid())
+ .setTopics(Arrays.asList(
+ new
AssignReplicasToDirsResponseData.TopicData()
+ .setName("bar")
+ .setPartitions(Arrays.asList(
+ new
AssignReplicasToDirsResponseData.PartitionData()
+
.setPartitionIndex(2)
+
.setErrorCode(Errors.UNKNOWN_TOPIC_OR_PARTITION.code()),
+ new
AssignReplicasToDirsResponseData.PartitionData()
+
.setPartitionIndex(8)
Review Comment:
Shouldn't this be 80 (due to line 1201)? Or the request and response tests
are not connected with each another?
##########
clients/src/main/resources/common/message/BrokerHeartbeatRequest.json:
##########
@@ -18,7 +18,7 @@
"type": "request",
"listeners": ["controller"],
"name": "BrokerHeartbeatRequest",
- "validVersions": "0",
+ "validVersions": "0-1",
Review Comment:
For my understanding, is there a benefit to updating the API version when
you are adding a tagged (optional) field only?
##########
metadata/src/test/java/org/apache/kafka/controller/PartitionChangeBuilderTest.java:
##########
@@ -426,7 +435,7 @@ public void testUncleanLeaderElection() {
.setIsr(Arrays.asList(1))
.setLeader(1)
.setLeaderRecoveryState(LeaderRecoveryState.RECOVERING.value()),
- PARTITION_CHANGE_RECORD.highestSupportedVersion()
+ (short) 0 // TODO test against highest supported version
Review Comment:
```suggestion
(short) 0 // TODO test against highest supported version
```
##########
metadata/src/test/java/org/apache/kafka/controller/PartitionChangeBuilderTest.java:
##########
@@ -411,7 +420,7 @@ public void testUncleanLeaderElection() {
.setIsr(Arrays.asList(2))
.setLeader(2)
.setLeaderRecoveryState(LeaderRecoveryState.RECOVERING.value()),
- PARTITION_CHANGE_RECORD.highestSupportedVersion()
+ (short) 0 // TODO test against highest supported version
Review Comment:
```suggestion
(short) 0 // TODO test against highest supported version
```
--
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
