sravani-revuri commented on code in PR #11022:
URL: https://github.com/apache/ozone/pull/11022#discussion_r3785601062
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestTransferLeadershipShell.java:
##########
@@ -100,29 +107,53 @@ public void testOmTransfer() throws Exception {
assertOMResetPriorities();
}
- @Test
- public void testScmTransfer() throws Exception {
- StorageContainerManager oldLeader = getScmLeader(cluster);
- List<StorageContainerManager> scmList = new ArrayList<>(cluster.
- getStorageContainerManagersList());
- assertThat(scmList).contains(oldLeader);
- scmList.remove(oldLeader);
- StorageContainerManager newLeader = scmList.get(0);
+ static Stream<Named<Function<StorageContainerManager, String>>>
scmTargetIds() {
+ return Stream.of(
+ Named.of("scmId", StorageContainerManager::getScmId),
+ Named.of("nodeId", StorageContainerManager::getSCMNodeId));
+ }
- String[] args1 = {"scm", "transfer", "-n", newLeader.getScmId()};
- ozoneAdmin.execute(args1);
+ @ParameterizedTest
+ @MethodSource("scmTargetIds")
+ public void testScmTransfer(Function<StorageContainerManager, String>
targetId) throws Exception {
+ StorageContainerManager newLeader = pickScmFollower();
+
+ String[] args = {"scm", "transfer", "-n", targetId.apply(newLeader)};
+ ozoneAdmin.execute(args);
cluster.waitForClusterToBeReady();
assertEquals(newLeader, getScmLeader(cluster));
assertSCMResetPriorities();
+ }
- oldLeader = getScmLeader(cluster);
- String[] args3 = {"scm", "transfer", "-r"};
- ozoneAdmin.execute(args3);
+ @Test
+ public void testScmTransferToRandomFollower() throws Exception {
+ StorageContainerManager oldLeader = getScmLeader(cluster);
+
+ String[] args = {"scm", "transfer", "-r"};
+ ozoneAdmin.execute(args);
cluster.waitForClusterToBeReady();
assertNotSame(oldLeader, getScmLeader(cluster));
assertSCMResetPriorities();
}
+ @Test
+ public void testScmTransferToUnknownIdFails() {
Review Comment:
You could also assert that the error message contains the invalid id
(no-such-scm). That ensures the failure is due to the bad id.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1019,6 +1020,23 @@ public void transferLeadership(String newLeaderId)
SCMAction.TRANSFER_LEADERSHIP, auditMap));
}
+ /** Resolves the transfer target, which may be given as an SCM UUID or as a
configured SCM node id. */
+ private RaftPeerId resolveTargetPeerId(String newLeaderId, RaftGroup group) {
+ final RaftPeerId peerId = RaftPeerId.valueOf(newLeaderId);
+ if (group.getPeer(peerId) != null) {
+ return peerId;
+ }
+ // Raft peer ids are SCM UUIDs, so a node id only matches through its
configured Ratis address.
+ return scm.getSCMHANodeDetails().getAllNodeDetails().stream()
Review Comment:
the code in this return is a bit hard to read with the nested stream nodeid
and peer logic. would it be better to replace this with a simple for-loop,
similar to getPrimordialNode() ?
something like this:
```
for each node in getAllNodeDetails()
if newLeaderId != node.nodeId then continue
for each peer in group
if peer.address == node.ratisAddress then return peer.id
return peerId // unmatched
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
