Skip to content

HDDS-15371. [DiskBalancer] DiskBalancer should use monotonic time for delayed replica deletion.#10364

Merged
adoroszlai merged 3 commits into
apache:masterfrom
slfan1989:HDDS-15371
May 27, 2026
Merged

HDDS-15371. [DiskBalancer] DiskBalancer should use monotonic time for delayed replica deletion.#10364
adoroszlai merged 3 commits into
apache:masterfrom
slfan1989:HDDS-15371

Conversation

@slfan1989
Copy link
Copy Markdown
Contributor

@slfan1989 slfan1989 commented May 26, 2026

What changes were proposed in this pull request?

DiskBalancer uses System.currentTimeMillis() to schedule and check delayed deletion of old replicas after a successful container move.

The delayed deletion is based on a relative delay configured by replicaDeletionDelay. Since System.currentTimeMillis() is wall-clock time, it can be affected by system clock changes such as NTP adjustment, manual time changes, or VM time drift.

If the system clock moves forward, old replicas may become eligible for deletion earlier than expected. If the system clock moves backward, old replicas may remain in the pending deletion queue longer than expected.

DiskBalancer already uses Time.monotonicNow() for other relative-time logic, such as bandwidth throttling with nextAvailableTime. The pending deletion delay should follow the same pattern and use monotonic time for both deadline calculation and expiration checks.

This makes delayed replica deletion independent of wall-clock changes and consistent with other DiskBalancer delay/elapsed-time logic.

What is the link to the Apache JIRA

HDDS-15371. DiskBalancer should use monotonic time for delayed replica deletion.

How was this patch tested?

Exists Unit Test.

@slfan1989 slfan1989 changed the title HDDS-15371. DiskBalancer should use monotonic time for delayed replica deletion. HDDS-15371. [DiskBalancer] DiskBalancer should use monotonic time for delayed replica deletion. May 26, 2026
Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @slfan1989 for the patch.

Can we use Clock instead of hard-coded time source? (MonotonicClock in prod, TestClock in test to avoid the need for sleep (waitFor).)

@slfan1989
Copy link
Copy Markdown
Contributor Author

Thanks @slfan1989 for the patch.

Can we use Clock instead of hard-coded time source? (MonotonicClock in prod, TestClock in test to avoid the need for sleep (waitFor).)

@adoroszlai Thanks for the suggestion. I have updated the implementation to inject a Clock: SlidingWindow.MonotonicClock is used in production, and TestClock is used in the test. This removes the real-time wait from testOldReplicaDelayedDeletion, as the test can now advance the clock directly. Please take another look when convenient.

Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @slfan1989 for updating the patch.

Before:

[INFO] Tests run: 56, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 28.32 s -- in org.apache.hadoop.ozone.container.diskbalancer.TestDiskBalancerTask

After:

[INFO] Tests run: 56, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 11.95 s -- in org.apache.hadoop.ozone.container.diskbalancer.TestDiskBalancerTask

Comment on lines -637 to +650
pendingDeletionContainers.put(System.currentTimeMillis() + replicaDeletionDelay, container);
pendingDeletionContainers.put(clock.millis() + replicaDeletionDelay,
container);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This line got shorter, so there was no need to wrap. Please adjust line length in your IDE to 120 to avoid it in the future.

@adoroszlai adoroszlai merged commit 7e7c436 into apache:master May 27, 2026
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants