brandboat commented on code in PR #22531:
URL: https://github.com/apache/kafka/pull/22531#discussion_r3488265083
##########
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandTest.java:
##########
@@ -194,4 +198,60 @@ private static void assertHumanReadable(String output) {
assertTrue(lastCaughtUpTimestamp.contains("ms ago"));
assertTrue(lastCaughtUpTimestampValue.matches("\\d*"));
}
+
+ /**
+ * Verify that add-controller and remove-controller work using only the
+ * controller node ID (no directory ID or endpoints required from the
user).
+ */
+ @ClusterTest(types = {Type.KRAFT}, controllers = 2, standalone = true)
+ public void testAddAndRemoveControllerByIdSuccessful(ClusterInstance
cluster) throws Exception {
Review Comment:
> Could we addVoter with [node ID, directory ID] or [node ID, endpoint] ?
Sure, I've refined the test case, please check
`KafkaRaftClientReconfigTest#testAddVoterDerivesMissingDirectoryIdAndEndpoints`
> If [node ID, directory ID] or [node ID, endpoint] doesn't match with each
other, ex: wrong directory ID/endpoint to the node, what will happen? Could we
also add tests for them?
This should behave the same in both v1 and v2 as I didn't do validation if
user explicitly specify both directory and endpoints, and should make
AddVoterHandler returns
REQUEST_TIMED_OUT[[1](https://github.com/apache/kafka/blob/1d953172b99306e5c31982a08a58ecc90130c96d/raft/src/main/java/org/apache/kafka/raft/LeaderState.java#L419)][[2](https://github.com/apache/kafka/blob/1d953172b99306e5c31982a08a58ecc90130c96d/raft/src/main/java/org/apache/kafka/raft/internals/AddVoterHandler.java#L294)].
Regarding the test, I currently have the integration test below, but I think
this behavior is better covered by a unit test. I'll try to figure out how to
implement it as a unit test tomorrow.
```
@ClusterTest(controllers = 3, standalone = true)
public void testAddRaftVoterWithMismatchedDirectoryId(ClusterInstance
clusterInstance) throws Exception {
for (boolean usingBootstrapControllers : List.of(false, true)) {
try (Admin admin = Admin.create(adminConfig(clusterInstance,
usingBootstrapControllers))) {
QuorumInfo quorumInfo =
admin.describeMetadataQuorum().quorumInfo().get();
QuorumInfo.ReplicaState observer =
controllerObserver(clusterInstance, quorumInfo);
Uuid anotherObserverDirectoryId =
quorumInfo.observers().stream()
.filter(replica -> replica.replicaId() !=
observer.replicaId())
.map(QuorumInfo.ReplicaState::replicaDirectoryId)
.filter(directoryId ->
!directoryId.equals(observer.replicaDirectoryId()))
.findFirst()
.orElseGet(Uuid::randomUuid);
TestUtils.assertFutureThrowsWithMessageMatching(
TimeoutException.class,
admin.addRaftVoter(
observer.replicaId(),
anotherObserverDirectoryId,
Set.of(controllerEndpoint(clusterInstance,
observer.replicaId())),
new AddRaftVoterOptions().timeoutMs(5_000)
).all(),
"Aborted add voter operation for .* since it is lagging
behind"
);
assertFalse(voterIds(admin.describeMetadataQuorum().quorumInfo().get()).contains(observer.replicaId()));
}
}
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
