HDDS-10314. Speed up TestDatanodeHddsVolumeFailureDetection#10819
HDDS-10314. Speed up TestDatanodeHddsVolumeFailureDetection#10819echonesis wants to merge 1 commit into
Conversation
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for working on this! @echonesis Reusing one cluster per schema version looks like a nice runtime improvement.
I left a few questions below, mainly about whether the schema V2 restore leaves any state behind between tests and whether the shorter node-detection intervals have enough CI headroom. These are non-blocking from my side, and I'd appreciate other reviewers’ thoughts.
Also, since this moves the suite onto a shared cluster with tighter timing, I think it might be worth triggering the flaky-test-check workflow on your fork for this branch. A clean repeated run linked in the PR description would make it easy to merge this with confidence.
| StorageVolume volume = volumeSet.getFailedVolumesList().get(0); | ||
| volume.setState(VolumeState.NORMAL); | ||
| volume.start(); | ||
| volumeSet.setVolumeMapForTesting( |
There was a problem hiding this comment.
Would restarting the V2 datanode also be a simpler way to avoid carrying this state into the next test? None of this breaks the current tests, but it is a contaminated state that future tests could trip over.
If you prefer to keep the in-place restore for speed, a short comment documenting that the volume intentionally stays in failedVolumeMap (and why that is safe) would help the next reader.
There was a problem hiding this comment.
The in-place restore was originally intended as a test optimization. Unlike schema V3, schema V2 uses per-container DBs, so restoring the volume to the active map was sufficient for the existing tests and avoided a datanode restart.
However, I agree that leaving it in failedVolumeMap creates an unnecessarily contaminated state.
Restarting the V2 datanode is cleaner and still preserves the main benefit of reusing the cluster.
I’ll update restoreVolume() to restart the datanode for both schema versions and remove the in-place V2 restoration logic.
| ozoneConfig.setTimeDuration(HDDS_HEARTBEAT_INTERVAL, 100, TimeUnit.MILLISECONDS); | ||
| ozoneConfig.setTimeDuration(OZONE_SCM_HEARTBEAT_PROCESS_INTERVAL, 100, TimeUnit.MILLISECONDS); | ||
| ozoneConfig.setTimeDuration(OZONE_SCM_STALENODE_INTERVAL, 1, TimeUnit.SECONDS); | ||
| ozoneConfig.setTimeDuration(OZONE_SCM_DEADNODE_INTERVAL, 2, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Are the 1s stale and 2s dead intervals needed for the restart flow? I wonder whether they could be tight on a busy CI runner. If slightly larger values (for example stale 3s / dead 6s) still keep the runtime win, they would leave more headroom.
There was a problem hiding this comment.
Yes, the stale interval is used by restartHddsDatanode, which waits for SCM to stop considering the datanode healthy before starting it again.
The dead interval was set alongside it to satisfy the required stale/dead relationship. I agree that 1s/2s leaves little headroom on a busy CI runner.
I’ll change them to 3s/6s; this should still preserve the runtime improvement from reusing the cluster.
| * As we test the helper method directly, we cannot test for schemas older than V3. | ||
| * | ||
| * @param schemaV3 | ||
| * @throws Exception |
There was a problem hiding this comment.
nit: the @param schemaV3 was removed but the bare @throws Exception remains. This javadoc could drop it too.
There was a problem hiding this comment.
Thanks for pointing this out!
I’ll remove the bare @throws Exception tag as well.
|
Thanks @chihsuan for the review. I tested the 3s/6s intervals together with restarting the V2 datanode locally. The runtime increased to about 121 seconds, which removes most of the intended speedup. There are roughly six datanode restarts across the V2 and V3 invocations. Since To preserve the purpose of this change, I propose keeping the safer 3s/6s intervals but restoring V2 in place. I’ll add a comment explicitly documenting that the V2 volume intentionally remains in failedVolumeMap, that subsequent tests use the restored active volume, and that the cluster is closed after the parameterized invocation. V3 will continue to restart because its shared DB is closed after a volume failure. What do you think? |
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for testing the combination and sharing the numbers. @echonesis
To preserve the purpose of this change, I propose keeping the safer 3s/6s intervals but restoring V2 in place. I’ll add a comment explicitly documenting that the V2 volume intentionally remains in failedVolumeMap, that subsequent tests use the restored active volume, and that the cluster is closed after the parameterized invocation. V3 will continue to restart because its shared DB is closed after a volume failure.
What do you think?
Sounds good and makes sense to me.
2 participants
What changes were proposed in this pull request?
This PR reduces the execution time of
TestDatanodeHddsVolumeFailureDetectionby reusing oneMiniOzoneClusterfor each container schema version instead of creating a new cluster for every test method.The test class is converted to a JUnit parameterized class covering schema V2 and schema V3. After each test, the failed volume is restored so that the cluster can be reused safely:
Generated-by: Codex (GPT-5)
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10314
How was this patch tested?
Local Test
GitHub Actions CI: https://github.com/echonesis/ozone/actions/runs/29737666866
Screenshot:
