Copilot commented on code in PR #11506:
URL: https://github.com/apache/ozone/pull/11506#discussion_r4255987605
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestFailureHandlingByClient.java:
##########
@@ -159,8 +162,28 @@ public void restartDownDataNodes() throws Exception {
for (DatanodeDetails dataNode : restartDataNodes) {
cluster.restartHddsDatanode(dataNode, false);
}
- restartDataNodes.clear();
cluster.waitForClusterToBeReady();
+ // A restarted datanode can take tens of seconds to rejoin its Ratis
groups, while SCM keeps allocating
+ // blocks on those pipelines. Wait for every group on it to have a leader
before the next test writes.
+ for (DatanodeDetails dataNode : restartDataNodes) {
+ RaftServer server = assertInstanceOf(XceiverServerRatis.class,
+
cluster.getHddsDatanode(dataNode).getDatanodeStateMachine().getContainer().getWriteChannel()).getServer();
+ GenericTestUtils.waitFor(() -> allGroupsHaveLeader(server), 500,
120_000);
+ }
+ restartDataNodes.clear();
+ }
+
+ private static boolean allGroupsHaveLeader(RaftServer server) {
+ try {
+ for (RaftGroupId groupId : server.getGroupIds()) {
+ if (server.getDivision(groupId).getInfo().getLeaderId() == null) {
Review Comment:
`server.getDivision(groupId)` can potentially return `null` (e.g., if the
division is not yet created or is concurrently being torn down), which would
cause a `NullPointerException` and break the wait behavior. Consider storing
the division in a local variable and returning `false` when it is `null` (so
the wait continues) rather than throwing.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestFailureHandlingByClient.java:
##########
@@ -159,8 +162,28 @@ public void restartDownDataNodes() throws Exception {
for (DatanodeDetails dataNode : restartDataNodes) {
cluster.restartHddsDatanode(dataNode, false);
}
- restartDataNodes.clear();
cluster.waitForClusterToBeReady();
+ // A restarted datanode can take tens of seconds to rejoin its Ratis
groups, while SCM keeps allocating
+ // blocks on those pipelines. Wait for every group on it to have a leader
before the next test writes.
+ for (DatanodeDetails dataNode : restartDataNodes) {
+ RaftServer server = assertInstanceOf(XceiverServerRatis.class,
+
cluster.getHddsDatanode(dataNode).getDatanodeStateMachine().getContainer().getWriteChannel()).getServer();
+ GenericTestUtils.waitFor(() -> allGroupsHaveLeader(server), 500,
120_000);
Review Comment:
The polling interval and timeout are magic numbers. To make future tuning
easier (and to keep consistency with other tests), consider extracting these
values into named constants (e.g., `RATIS_LEADER_WAIT_POLL_MS`,
`RATIS_LEADER_WAIT_TIMEOUT_MS`) or reusing an existing test timeout constant if
one exists in this class/suite.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestFailureHandlingByClient.java:
##########
@@ -159,8 +162,28 @@ public void restartDownDataNodes() throws Exception {
for (DatanodeDetails dataNode : restartDataNodes) {
cluster.restartHddsDatanode(dataNode, false);
}
- restartDataNodes.clear();
cluster.waitForClusterToBeReady();
+ // A restarted datanode can take tens of seconds to rejoin its Ratis
groups, while SCM keeps allocating
+ // blocks on those pipelines. Wait for every group on it to have a leader
before the next test writes.
+ for (DatanodeDetails dataNode : restartDataNodes) {
+ RaftServer server = assertInstanceOf(XceiverServerRatis.class,
+
cluster.getHddsDatanode(dataNode).getDatanodeStateMachine().getContainer().getWriteChannel()).getServer();
+ GenericTestUtils.waitFor(() -> allGroupsHaveLeader(server), 500,
120_000);
+ }
+ restartDataNodes.clear();
+ }
+
+ private static boolean allGroupsHaveLeader(RaftServer server) {
+ try {
+ for (RaftGroupId groupId : server.getGroupIds()) {
+ if (server.getDivision(groupId).getInfo().getLeaderId() == null) {
+ return false;
+ }
+ }
+ return true;
+ } catch (IOException e) {
+ return false;
+ }
}
Review Comment:
`allGroupsHaveLeader` returns `true` when `server.getGroupIds()` is empty
(loop never runs). Right after a restart, this can allow the test to proceed
even if the datanode hasn’t rejoined/initialized any Ratis groups yet, which
undermines the intent of the wait. Consider explicitly treating an empty
`groupIds` collection as “not ready” (return `false`) for these restarted
datanodes, or otherwise assert/require at least one group before checking
leader IDs.
--
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]
