You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "andyhuangdev (via GitHub)" <gi...@apache.org> on 2026/10/19 04:26:08 UTC
[PR] HDDS-17131. Preserve OM list when transferring mini-cluster leadership [ozone]
andyhuangdev opened a new pull request, #11620:
URL: https://github.com/apache/ozone/pull/11620
## What changes were proposed in this pull request?
MiniOzoneHAClusterImpl#transferOMLeadershipToAnotherNode removes the current leader from the cluster's backing OM list. This changes OM indexes and causes cluster shutdown to skip the removed OM. The helper can also select a stopped OM as the transfer target.
This patch copies the OM list and selects a running OM other than the current leader. It throws an explicit exception if no eligible OM exists.
## What is the link to the Apache JIRA
[HDDS-17131](https://issues.apache.org/jira/browse/HDDS-17131)
## How was this patch tested?
Added parameterized regression coverage for 3 running OMs and 5 OMs with one stopped follower. The tests verify repeated leadership transfers, unchanged OM lists and indexes, running transfer targets, and cluster shutdown.
Both cases reproduced the defects before the fix and passed afterward:
mvn -pl :ozone-integration-test test \
-Dtest=TestMiniOzoneCluster#testTransferOMLeadershipPreservesCluster \
-DskipShade -DskipRecon -DskipDocs
./hadoop-ozone/dev-support/checks/checkstyle.sh also passed.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-17131. Preserve OM list when transferring mini-cluster leadership [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #11620:
URL: https://github.com/apache/ozone/pull/11620#issuecomment-6310436894
Thanks @andyhuangdev for the patch and @adoroszlai for the review.
@smengcl merged due to HDDS-17180 has been fixed.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-17131. Preserve OM list when transferring mini-cluster leadership [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #11620:
URL: https://github.com/apache/ozone/pull/11620#discussion_r4332085597
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHAFollowerReadWithAllRunning.java:
##########
@@ -589,8 +589,13 @@ void testClientWithFollowerReadDisabled() throws Exception {
assertEquals(leaderOMNodeId, leaderProxyProvider.getCurrentProxyOMNodeId());
// Try to transfer leadership
+ List<OzoneManager> originalOMs = new ArrayList<>(getCluster().getOzoneManagersList());
Review Comment:
I think it's better to do the defensive copy in the `getOzoneManagersList` method directly. The `MiniOzoneHAService` already have mutation methods in `addInstance` `removeInstance`, etc. Therefore, I think I was assuming that `getOzoneManagersList` returns a defensive copy. This also applied to the other files.
I currently see no reason why we want to return the actual list, which can be misused.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-17131. Preserve OM list when transferring mini-cluster leadership [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #11620:
URL: https://github.com/apache/ozone/pull/11620#discussion_r4325452508
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestMiniOzoneCluster.java:
##########
@@ -114,6 +115,43 @@ public void testStartMultipleDatanodes() throws Exception {
}
}
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ void testTransferOMLeadershipPreservesCluster(boolean stopFollower) throws Exception {
+ MiniOzoneHAClusterImpl haCluster = MiniOzoneCluster.newHABuilder(conf)
+ .setOMServiceId("om-service")
+ .setNumOfOzoneManagers(stopFollower ? 5 : 3)
+ .build();
Review Comment:
This doubles execution time of the test class. I would like to avoid that.
Before:
```
Tests run: 17, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 76.83 s -- in org.apache.hadoop.ozone.TestMiniOzoneCluster
```
After:
```
Tests run: 19, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 145.0 s -- in org.apache.hadoop.ozone.TestMiniOzoneCluster
```
`transferOMLeadershipToAnotherNode` only exists in `MiniOzoneHAClusterImpl`, i.e. it's test functionality. Is it possible to add assertions about the fixed behavior in an existing test where it's already used, with less increase of runtime?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestMiniOzoneCluster.java:
##########
@@ -114,6 +115,43 @@ public void testStartMultipleDatanodes() throws Exception {
}
}
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ void testTransferOMLeadershipPreservesCluster(boolean stopFollower) throws Exception {
+ MiniOzoneHAClusterImpl haCluster = MiniOzoneCluster.newHABuilder(conf)
+ .setOMServiceId("om-service")
+ .setNumOfOzoneManagers(stopFollower ? 5 : 3)
+ .build();
+ cluster = haCluster;
+ cluster.waitForClusterToBeReady();
+ haCluster.waitForLeaderOM();
+ List<OzoneManager> originalOMs = new ArrayList<>(haCluster.getOzoneManagersList());
+ OzoneManager leader = haCluster.getOMLeader();
+ if (stopFollower) {
+ OzoneManager follower = originalOMs.stream().filter(om -> om != leader).findFirst().get();
+ haCluster.stopOzoneManager(follower.getOMNodeId());
+ }
+
+ try {
+ // Repeated transfers must not remove any OM or change its index.
+ for (int i = 0; i < 3; i++) {
+ OzoneManager oldLeader = haCluster.getOMLeader();
+ OzoneManager newLeader = haCluster.transferOMLeadershipToAnotherNode(oldLeader);
+ assertNotEquals(oldLeader, newLeader);
+ assertTrue(newLeader.isRunning());
+ assertEquals(originalOMs, haCluster.getOzoneManagersList());
+ for (int j = 0; j < originalOMs.size(); j++) {
+ assertEquals(originalOMs.get(j), haCluster.getOzoneManager(j));
+ }
+ }
+
+ haCluster.stop();
Review Comment:
cluster is not stopped in case of failures/errors.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-17131. Preserve OM list when transferring mini-cluster leadership [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 merged PR #11620:
URL: https://github.com/apache/ozone/pull/11620
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-17131. Preserve OM list when transferring mini-cluster leadership [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #11620:
URL: https://github.com/apache/ozone/pull/11620#discussion_r4332085597
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHAFollowerReadWithAllRunning.java:
##########
@@ -589,8 +589,13 @@ void testClientWithFollowerReadDisabled() throws Exception {
assertEquals(leaderOMNodeId, leaderProxyProvider.getCurrentProxyOMNodeId());
// Try to transfer leadership
+ List<OzoneManager> originalOMs = new ArrayList<>(getCluster().getOzoneManagersList());
Review Comment:
I think it's better to do the defensive copy in the `getOzoneManagersList` method directly. The `MiniOzoneHAService` already have mutation methods in `addInstance` `removeInstance`, etc. That was my assumption. This also applied to the other files.
I currently see no reason why we want to return the actual list, which can be misused.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-17131. Preserve OM list when transferring mini-cluster leadership [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #11620:
URL: https://github.com/apache/ozone/pull/11620#issuecomment-6298293946
Before:
```
[INFO] Tests run: 14, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 37.93 s -- in org.apache.hadoop.ozone.om.TestOzoneManagerHAFollowerReadWithAllRunning
[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 162.2 s -- in org.apache.hadoop.ozone.om.TestOzoneManagerHAFollowerReadWithStoppedNodes
```
After:
```
[INFO] Tests run: 14, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 49.60 s -- in org.apache.hadoop.ozone.om.TestOzoneManagerHAFollowerReadWithAllRunning
[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 170.4 s -- in org.apache.hadoop.ozone.om.TestOzoneManagerHAFollowerReadWithStoppedNodes
```
Thanks @andyhuangdev for updating the patch, this looks more reasonable.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-17131. Preserve OM list when transferring mini-cluster leadership [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #11620:
URL: https://github.com/apache/ozone/pull/11620#discussion_r4332085597
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHAFollowerReadWithAllRunning.java:
##########
@@ -589,8 +589,13 @@ void testClientWithFollowerReadDisabled() throws Exception {
assertEquals(leaderOMNodeId, leaderProxyProvider.getCurrentProxyOMNodeId());
// Try to transfer leadership
+ List<OzoneManager> originalOMs = new ArrayList<>(getCluster().getOzoneManagersList());
Review Comment:
I think it's better to do the defensive copy in the `getOzoneManagersList` or `getServices` method directly. The `MiniOzoneHAService` already have mutation methods in `addInstance` `removeInstance`, etc. Therefore, I think I was assuming that `getOzoneManagersList` returns a defensive copy. This also applied to the other files.
I currently see no reason why we want to return the actual list, which can be misused.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-17131. Preserve OM list when transferring mini-cluster leadership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #11620:
URL: https://github.com/apache/ozone/pull/11620#issuecomment-6310347238
CI failure is a known issue that is fixed in master: https://github.com/apache/ozone/actions/runs/39532079401/job/119071391032?pr=11620
PR branch CI is green: https://github.com/andyhuangdev/ozone/actions/runs/39532074958
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org