You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "chihsuan (via GitHub)" <gi...@apache.org> on 2026/06/06 11:26:25 UTC
[PR] HDDS-10310. Speed up TestOMRatisSnapshots [ozone]
chihsuan opened a new pull request, #10451:
URL: https://github.com/apache/ozone/pull/10451
## What changes were proposed in this pull request?
`TestOMRatisSnapshots` takes ~575s locally (12 tests: 6 active methods x 2 parameters since HDDS-14721 made the class parameterized). Profiling the run shows a large share of the time is fixed sleeps and oversized cluster setup rather than the code under test. This PR removes the dead time without weakening any assertion:
1. **Remove the per-key `Thread.sleep(100)` in `writeKeysToIncreaseLogIndex`.** Each `createKey()` is a synchronous RPC producing 2 Ratis transactions, so the loop converges without sleeping; callers only require reaching at least the target index. The sleep was introduced without documented rationale in the HDDS-3741 refactor (the original HDDS-1649 loop had no sleep). This alone was ~40s of pure sleep per parameter run.
2. **Replace/remove three hardcoded `Thread.sleep(5000)`.** Two were redundant (immediately followed by 30s-polling assertions); the third now polls for the last written key to appear in the follower's key table, the same pattern already used elsewhere in this class.
3. **Use a single datanode.** All key writes in this class use `ReplicationFactor.ONE`, so 3 DNs only add startup/teardown cost for each of the 12 cluster instances. (`setNumDatanodes` returns the base builder type, hence the builder chain is split.)
4. **Fix a pre-existing race in `testInstallOldCheckpointFailure` exposed by (1).** The test reads `followerTermIndex` and asserts a log message containing that exact TermIndex, but trailing Ratis applies can advance the index between the test's read and `installCheckpoint()`'s internal read (observed: expected `i:202`, OM logged `i:203`). The old per-key sleep happened to mask this. The fix waits for the follower's applied index to reach the leader's (quiescence) before reading, instead of relying on sleep timing.
Measured locally (M-series MacBook): 574.3s before, 483.9s / 525.7s on two full runs after (run-to-run variance from machine load), all 12 tests passing.
Follow-up (separate Jira, to be filed under HDDS-9000): limit the HDDS-14721 class-level parameterization to the tests that actually exercise the checkpoint transfer format, which roughly halves the remaining cost.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10310
## How was this patch tested?
Full `TestOMRatisSnapshots` class run locally with both parameters:
```
mvn -pl hadoop-ozone/integration-test test -Dtest=TestOMRatisSnapshots
Tests run: 12, Failures: 0, Errors: 0, Skipped: 0
```
Repeated full-class runs to check for flakiness (a third run is in progress; will update before marking ready for review). Per-test comparison against the baseline shows the affected tests improving consistently, e.g. `testInstallSnapshotWithClientWrite` 50-55s -> 33-39s, `testInstallSnapshotWithClientRead` 44s -> 26-37s, `testInstallOldCheckpointFailure` 33-37s -> 23-28s. `checkstyle`, `rat`, and `author` checks pass on the module.
--
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-10310. Speed up TestOMRatisSnapshots [ozone]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on PR #10451:
URL: https://github.com/apache/ozone/pull/10451#issuecomment-4656468438
Merged. Thanks for the contribution @chihsuan
--
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-10310. Speed up TestOMRatisSnapshots [ozone]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #10451:
URL: https://github.com/apache/ozone/pull/10451#discussion_r3370645895
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -805,6 +804,24 @@ public void testInstallSnapshotWithClientWrite() throws Exception {
assertLogCapture(logCapture, msg);
assertLogCapture(logCapture, "Install Checkpoint is finished");
+ // All newKeys writes have completed (writeFuture.get() above), so the
+ // leader must already contain them.
+ OMMetadataManager leaderOmMetaMgr = leaderOM.getMetadataManager();
+ for (String key : newKeys) {
+ assertNotNull(leaderOmMetaMgr.getKeyTable(
+ TEST_BUCKET_LAYOUT)
+ .get(leaderOmMetaMgr.getOzoneKey(volumeName, bucketName, key)));
+ }
Review Comment:
Nit: we can move this loop assertion right after `List<String> newKeys = writeFuture.get();` because they are logically co-related: we write new keys, and right after we verify all the new key are there.
--
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-10310. Speed up TestOMRatisSnapshots [ozone]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #10451:
URL: https://github.com/apache/ozone/pull/10451#discussion_r3370872034
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -169,11 +169,13 @@ public void init(TestInfo testInfo) throws Exception {
clientConfig.setRpcTimeOut(TimeUnit.SECONDS.toMillis(5));
conf.setFromObject(clientConfig);
- cluster = MiniOzoneCluster.newHABuilder(conf)
- .setOMServiceId("om-service-test1")
+ MiniOzoneHAClusterImpl.Builder clusterBuilder =
+ MiniOzoneCluster.newHABuilder(conf);
+ clusterBuilder.setOMServiceId("om-service-test1")
.setNumOfOzoneManagers(NUM_OF_OMS)
.setNumOfActiveOMs(2)
- .build();
+ .setNumDatanodes(1);
+ cluster = clusterBuilder.build();
Review Comment:
Nit:
cluster = MiniOzoneCluster.newHABuilder(conf)
setOMServiceId("om-service-test1")
.setNumOfOzoneManagers(NUM_OF_OMS)
.setNumOfActiveOMs(2)
.setNumDatanodes(1)
.build()
--
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-10310. Speed up TestOMRatisSnapshots [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
chihsuan commented on code in PR #10451:
URL: https://github.com/apache/ozone/pull/10451#discussion_r3371632728
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -805,6 +804,24 @@ public void testInstallSnapshotWithClientWrite() throws Exception {
assertLogCapture(logCapture, msg);
assertLogCapture(logCapture, "Install Checkpoint is finished");
+ // All newKeys writes have completed (writeFuture.get() above), so the
+ // leader must already contain them.
+ OMMetadataManager leaderOmMetaMgr = leaderOM.getMetadataManager();
+ for (String key : newKeys) {
+ assertNotNull(leaderOmMetaMgr.getKeyTable(
+ TEST_BUCKET_LAYOUT)
+ .get(leaderOmMetaMgr.getOzoneKey(volumeName, bucketName, key)));
+ }
Review Comment:
Good point, I'll move the leader key check right after `writeFuture.get()`. Thanks!
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -805,6 +804,24 @@ public void testInstallSnapshotWithClientWrite() throws Exception {
assertLogCapture(logCapture, msg);
assertLogCapture(logCapture, "Install Checkpoint is finished");
+ // All newKeys writes have completed (writeFuture.get() above), so the
+ // leader must already contain them.
+ OMMetadataManager leaderOmMetaMgr = leaderOM.getMetadataManager();
+ for (String key : newKeys) {
+ assertNotNull(leaderOmMetaMgr.getKeyTable(
+ TEST_BUCKET_LAYOUT)
+ .get(leaderOmMetaMgr.getOzoneKey(volumeName, bucketName, key)));
+ }
+
+ // Wait for the follower to apply everything the leader has applied; all
+ // writes have completed on the leader, so after this no further snapshot
+ // install (and DB reload) can occur and the follower DB reads below are
+ // safe from "Rocks Database is closed" races.
+ long leaderApplied = leaderOM.getOmRatisServer()
+ .getLastAppliedTermIndex().getIndex();
+ GenericTestUtils.waitFor(() -> followerOM.getOmRatisServer()
+ .getLastAppliedTermIndex().getIndex() >= leaderApplied, 100, 30_000);
+
Review Comment:
Agreed, that's exactly the approach here. The first sleep's waitFor was already there (the leaderOMSnapshotIndex - 1 check right below it), so removing the sleep is safe.
The second sleep is now replaced by the leaderApplied waitFor, which matches your snippet. I kept the timeout at `30_000` to stay consistent with the other waitFor calls in this file (since it returns as soon as the condition is met, the ceiling only affects the failure case.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -169,11 +169,13 @@ public void init(TestInfo testInfo) throws Exception {
clientConfig.setRpcTimeOut(TimeUnit.SECONDS.toMillis(5));
conf.setFromObject(clientConfig);
- cluster = MiniOzoneCluster.newHABuilder(conf)
- .setOMServiceId("om-service-test1")
+ MiniOzoneHAClusterImpl.Builder clusterBuilder =
+ MiniOzoneCluster.newHABuilder(conf);
+ clusterBuilder.setOMServiceId("om-service-test1")
.setNumOfOzoneManagers(NUM_OF_OMS)
.setNumOfActiveOMs(2)
- .build();
+ .setNumDatanodes(1);
+ cluster = clusterBuilder.build();
Review Comment:
I'd love to keep it as a single chain, but it won't work. The `setNumDatanodes` is only defined on the base `MiniOzoneCluster.Builder,` so calling it downgrades the static type to the base builder. After that, `.build()` resolves to the base `build()` which returns `MiniOzoneCluster`, not `MiniOzoneHAClusterImpl`.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -931,8 +941,6 @@ public void testInstallSnapshotWithClientRead() throws Exception {
.get(followerOMMetaMngr.getOzoneKey(volumeName, bucketName, key)));
}
- // Wait installation finish
- Thread.sleep(5000);
Review Comment:
For this one we don't really need a `waitFor`. The line right after is `assertLogCapture(...)`, which is itself a `waitFor` (polls up to `30s`). The sleep was purely redundant, so I just removed it.
--
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-10310. Speed up TestOMRatisSnapshots [ozone]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #10451:
URL: https://github.com/apache/ozone/pull/10451#discussion_r3370679204
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -805,6 +804,24 @@ public void testInstallSnapshotWithClientWrite() throws Exception {
assertLogCapture(logCapture, msg);
assertLogCapture(logCapture, "Install Checkpoint is finished");
+ // All newKeys writes have completed (writeFuture.get() above), so the
+ // leader must already contain them.
+ OMMetadataManager leaderOmMetaMgr = leaderOM.getMetadataManager();
+ for (String key : newKeys) {
+ assertNotNull(leaderOmMetaMgr.getKeyTable(
+ TEST_BUCKET_LAYOUT)
+ .get(leaderOmMetaMgr.getOzoneKey(volumeName, bucketName, key)));
+ }
+
+ // Wait for the follower to apply everything the leader has applied; all
+ // writes have completed on the leader, so after this no further snapshot
+ // install (and DB reload) can occur and the follower DB reads below are
+ // safe from "Rocks Database is closed" races.
+ long leaderApplied = leaderOM.getOmRatisServer()
+ .getLastAppliedTermIndex().getIndex();
+ GenericTestUtils.waitFor(() -> followerOM.getOmRatisServer()
+ .getLastAppliedTermIndex().getIndex() >= leaderApplied, 100, 30_000);
+
Review Comment:
I am thinking the logic is more clear and close back to the original test if we
1. for the first `Thread.sleep(5000);`, we do:
```
GenericTestUtils.waitFor(() -> {
return followerOM.getOmRatisServer().getLastAppliedTermIndex().getIndex()
>= leaderOMSnapshotIndex - 1;
}, 100, 50_000);
```
2. then for the second `Thread.sleep(5000);`, we do
```
long leaderApplied = leaderOM.getOmRatisServer()
.getLastAppliedTermIndex().getIndex();
GenericTestUtils.waitFor(() -> followerOM.getOmRatisServer()
.getLastAppliedTermIndex().getIndex() >= leaderApplied, 100, 50_000);
```
The core idea for the speedup is not to sleep the fixed 50 seconds but wait for up to 50 seconds and hopefully the operation finishes earlier.
--
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-10310. Speed up TestOMRatisSnapshots [ozone]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #10451:
URL: https://github.com/apache/ozone/pull/10451#discussion_r3370873659
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -931,8 +941,6 @@ public void testInstallSnapshotWithClientRead() throws Exception {
.get(followerOMMetaMngr.getOzoneKey(volumeName, bucketName, key)));
}
- // Wait installation finish
- Thread.sleep(5000);
Review Comment:
Same as above that we can do `GenericTestUtils.waitFor` to wait up to 50 seconds.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -169,11 +169,13 @@ public void init(TestInfo testInfo) throws Exception {
clientConfig.setRpcTimeOut(TimeUnit.SECONDS.toMillis(5));
conf.setFromObject(clientConfig);
- cluster = MiniOzoneCluster.newHABuilder(conf)
- .setOMServiceId("om-service-test1")
+ MiniOzoneHAClusterImpl.Builder clusterBuilder =
+ MiniOzoneCluster.newHABuilder(conf);
+ clusterBuilder.setOMServiceId("om-service-test1")
.setNumOfOzoneManagers(NUM_OF_OMS)
.setNumOfActiveOMs(2)
- .build();
+ .setNumDatanodes(1);
+ cluster = clusterBuilder.build();
Review Comment:
Nit:
cluster = MiniOzoneCluster.newHABuilder(conf)
setOMServiceId("om-service-test1")
.setNumOfOzoneManagers(NUM_OF_OMS)
.setNumOfActiveOMs(2)
.build();
.setNumDatanodes(1)
.build()
--
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-10310. Speed up TestOMRatisSnapshots [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
chihsuan commented on code in PR #10451:
URL: https://github.com/apache/ozone/pull/10451#discussion_r3373226246
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -805,6 +804,24 @@ public void testInstallSnapshotWithClientWrite() throws Exception {
assertLogCapture(logCapture, msg);
assertLogCapture(logCapture, "Install Checkpoint is finished");
+ // All newKeys writes have completed (writeFuture.get() above), so the
+ // leader must already contain them.
+ OMMetadataManager leaderOmMetaMgr = leaderOM.getMetadataManager();
+ for (String key : newKeys) {
+ assertNotNull(leaderOmMetaMgr.getKeyTable(
+ TEST_BUCKET_LAYOUT)
+ .get(leaderOmMetaMgr.getOzoneKey(volumeName, bucketName, key)));
+ }
Review Comment:
done, moved it up in 95a330d4117505d04adb5e4806b77d4372b32a57
--
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-10310. Speed up TestOMRatisSnapshots [ozone]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #10451:
URL: https://github.com/apache/ozone/pull/10451#discussion_r3370679204
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -805,6 +804,24 @@ public void testInstallSnapshotWithClientWrite() throws Exception {
assertLogCapture(logCapture, msg);
assertLogCapture(logCapture, "Install Checkpoint is finished");
+ // All newKeys writes have completed (writeFuture.get() above), so the
+ // leader must already contain them.
+ OMMetadataManager leaderOmMetaMgr = leaderOM.getMetadataManager();
+ for (String key : newKeys) {
+ assertNotNull(leaderOmMetaMgr.getKeyTable(
+ TEST_BUCKET_LAYOUT)
+ .get(leaderOmMetaMgr.getOzoneKey(volumeName, bucketName, key)));
+ }
+
+ // Wait for the follower to apply everything the leader has applied; all
+ // writes have completed on the leader, so after this no further snapshot
+ // install (and DB reload) can occur and the follower DB reads below are
+ // safe from "Rocks Database is closed" races.
+ long leaderApplied = leaderOM.getOmRatisServer()
+ .getLastAppliedTermIndex().getIndex();
+ GenericTestUtils.waitFor(() -> followerOM.getOmRatisServer()
+ .getLastAppliedTermIndex().getIndex() >= leaderApplied, 100, 30_000);
+
Review Comment:
I am thinking the logic is more clear and close back to the original test if we
1. for the first `Thread.sleep(5000);`, we do:
```
GenericTestUtils.waitFor(() -> {
return followerOM.getOmRatisServer().getLastAppliedTermIndex().getIndex()
>= leaderOMSnapshotIndex - 1;
}, 100, 50_000);
```
2. then for the second `Thread.sleep(5000);`, we do
```
long leaderApplied = leaderOM.getOmRatisServer()
.getLastAppliedTermIndex().getIndex();
GenericTestUtils.waitFor(() -> followerOM.getOmRatisServer()
.getLastAppliedTermIndex().getIndex() >= leaderApplied, 100, 50_000);
```
--
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-10310. Speed up TestOMRatisSnapshots [ozone]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia merged PR #10451:
URL: https://github.com/apache/ozone/pull/10451
--
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-10310. Speed up TestOMRatisSnapshots [ozone]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #10451:
URL: https://github.com/apache/ozone/pull/10451#discussion_r3370645895
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -805,6 +804,24 @@ public void testInstallSnapshotWithClientWrite() throws Exception {
assertLogCapture(logCapture, msg);
assertLogCapture(logCapture, "Install Checkpoint is finished");
+ // All newKeys writes have completed (writeFuture.get() above), so the
+ // leader must already contain them.
+ OMMetadataManager leaderOmMetaMgr = leaderOM.getMetadataManager();
+ for (String key : newKeys) {
+ assertNotNull(leaderOmMetaMgr.getKeyTable(
+ TEST_BUCKET_LAYOUT)
+ .get(leaderOmMetaMgr.getOzoneKey(volumeName, bucketName, key)));
+ }
Review Comment:
Nit: we can move this loop assertion right after `List<String> newKeys = writeFuture.get();` because they are logically co-related: we write new keys, and right after we verify all the new key are there
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -805,6 +804,24 @@ public void testInstallSnapshotWithClientWrite() throws Exception {
assertLogCapture(logCapture, msg);
assertLogCapture(logCapture, "Install Checkpoint is finished");
+ // All newKeys writes have completed (writeFuture.get() above), so the
+ // leader must already contain them.
+ OMMetadataManager leaderOmMetaMgr = leaderOM.getMetadataManager();
+ for (String key : newKeys) {
+ assertNotNull(leaderOmMetaMgr.getKeyTable(
+ TEST_BUCKET_LAYOUT)
+ .get(leaderOmMetaMgr.getOzoneKey(volumeName, bucketName, key)));
+ }
+
+ // Wait for the follower to apply everything the leader has applied; all
+ // writes have completed on the leader, so after this no further snapshot
+ // install (and DB reload) can occur and the follower DB reads below are
+ // safe from "Rocks Database is closed" races.
+ long leaderApplied = leaderOM.getOmRatisServer()
+ .getLastAppliedTermIndex().getIndex();
+ GenericTestUtils.waitFor(() -> followerOM.getOmRatisServer()
+ .getLastAppliedTermIndex().getIndex() >= leaderApplied, 100, 30_000);
+
Review Comment:
I am thinking the logic is more clear and close back to the original test if we
1. for the first `Thread.sleep(5000);`, we do:
GenericTestUtils.waitFor(() -> {
return followerOM.getOmRatisServer().getLastAppliedTermIndex().getIndex()
>= leaderOMSnapshotIndex - 1;
}, 100, 50_000);
2. then for the second `Thread.sleep(5000);`, we do
```
long leaderApplied = leaderOM.getOmRatisServer()
.getLastAppliedTermIndex().getIndex();
GenericTestUtils.waitFor(() -> followerOM.getOmRatisServer()
.getLastAppliedTermIndex().getIndex() >= leaderApplied, 100, 50_000);
```
--
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-10310. Speed up TestOMRatisSnapshots [ozone]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #10451:
URL: https://github.com/apache/ozone/pull/10451#discussion_r3370645895
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -805,6 +804,24 @@ public void testInstallSnapshotWithClientWrite() throws Exception {
assertLogCapture(logCapture, msg);
assertLogCapture(logCapture, "Install Checkpoint is finished");
+ // All newKeys writes have completed (writeFuture.get() above), so the
+ // leader must already contain them.
+ OMMetadataManager leaderOmMetaMgr = leaderOM.getMetadataManager();
+ for (String key : newKeys) {
+ assertNotNull(leaderOmMetaMgr.getKeyTable(
+ TEST_BUCKET_LAYOUT)
+ .get(leaderOmMetaMgr.getOzoneKey(volumeName, bucketName, key)));
+ }
Review Comment:
Nit: we can move this loop assertion to right after `List<String> newKeys = writeFuture.get();` because they are logically co-related: we write new keys, and right after we verify all the new key are there.
--
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