[GitHub] [ozone] sodonnel commented on pull request #3337: HDDS-6598. Add a BackgroundPipelineScrubber to scrub all pipelines.

[GitBox <gi...@apache.org>]

sodonnel commented on PR #3337:
URL: https://github.com/apache/ozone/pull/3337#issuecomment-1110773580

   These changes LGTM. Thanks for addressing my comments. I have one more suggestion, but we could do this in another Jira if you want.
   
   There are a couple of places in the new scrubber code and the existing scrubber code in PipelineManagerImpl, where it uses `Time.monotonicNow()` to decide if the Safemode interval has passed, or if a pipeline has been Closed long enough etc. The unit tests do not correctly test these scenarios, as we just set the time to zero so there is no delay, otherwise the tests would need sleep calls, which will make them slow.
   
   In ReplicationManager, we addressed this problem by injecting a Clock dependency. See `MonotonicClock` - if we inject this as a dependency to the scrubber code, then we can inject a `MonotonicClock` for runtime, but inject `TestClock` for tests. Then you can properly test the safemode delay by advancing the clock between check calls. Same for pipelines - we can check CLOSED ones are not removed before the delay, and then check they are scrubbed after the delay.
   
   In general, we should try to avoid calls to `Time.monotonicNow()` across the codebase, and instead inject a clock as a dependency to make the code more testable without sleeps.


-- 
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