You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/04/28 14:20:40 UTC
[GitHub] [ozone] sodonnel opened a new pull request, #3363: HDDS-6655. Use injected clocks in PipelineManagerImp and BackgroundPipelineScrubber to ease testing
sodonnel opened a new pull request, #3363:
URL: https://github.com/apache/ozone/pull/3363
## What changes were proposed in this pull request?
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 ALLOCATED 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.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6655
## How was this patch tested?
Modified tests.
--
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
[GitHub] [ozone] sodonnel commented on a diff in pull request #3363: HDDS-6655. Use injected clocks in PipelineManagerImp and BackgroundPipelineScrubber to ease testing
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3363:
URL: https://github.com/apache/ozone/pull/3363#discussion_r862100724
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelineManagerImpl.java:
##########
@@ -536,9 +543,9 @@ null, pipelineManager, new EventQueue(),
@Test
public void testScrubPipelines() throws Exception {
- // No timeout for pipeline scrubber.
+ // Allocated pipelines should not be scrubbed for 50 seconds.
conf.setTimeDuration(
- OZONE_SCM_PIPELINE_ALLOCATED_TIMEOUT, -1,
+ OZONE_SCM_PIPELINE_ALLOCATED_TIMEOUT, 50000,
TimeUnit.MILLISECONDS);
Review Comment:
Yea I agree, I just used what was there. I will change this.
--
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
[GitHub] [ozone] adoroszlai merged pull request #3363: HDDS-6655. Use injected clocks in PipelineManagerImp and BackgroundPipelineScrubber to ease testing
Posted by GitBox <gi...@apache.org>.
adoroszlai merged PR #3363:
URL: https://github.com/apache/ozone/pull/3363
--
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
[GitHub] [ozone] adoroszlai commented on a diff in pull request #3363: HDDS-6655. Use injected clocks in PipelineManagerImp and BackgroundPipelineScrubber to ease testing
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on code in PR #3363:
URL: https://github.com/apache/ozone/pull/3363#discussion_r861679875
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestBackgroundPipelineScrubber.java:
##########
@@ -43,19 +46,21 @@ public class TestBackgroundPipelineScrubber {
private SCMContext scmContext;
private PipelineManager pipelineManager;
private OzoneConfiguration conf;
+ private TestClock testClock;
@Before
public void setup() throws IOException {
+ testClock = new TestClock(Instant.now(), ZoneOffset.UTC);
this.scmContext = SCMContext.emptyContext();
this.pipelineManager = mock(PipelineManager.class);
doNothing().when(pipelineManager).scrubPipelines();
// no initial delay after exit safe mode
this.conf = new OzoneConfiguration();
- conf.set(HddsConfigKeys.HDDS_SCM_WAIT_TIME_AFTER_SAFE_MODE_EXIT, "0ms");
+ conf.set(HddsConfigKeys.HDDS_SCM_WAIT_TIME_AFTER_SAFE_MODE_EXIT, "50000ms");
Review Comment:
"no initial delay" comment no longer applies.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelineManagerImpl.java:
##########
@@ -536,9 +543,9 @@ null, pipelineManager, new EventQueue(),
@Test
public void testScrubPipelines() throws Exception {
- // No timeout for pipeline scrubber.
+ // Allocated pipelines should not be scrubbed for 50 seconds.
conf.setTimeDuration(
- OZONE_SCM_PIPELINE_ALLOCATED_TIMEOUT, -1,
+ OZONE_SCM_PIPELINE_ALLOCATED_TIMEOUT, 50000,
TimeUnit.MILLISECONDS);
Review Comment:
Maybe it's just me, but I prefer `50, TimeUnit.SECONDS` to `50000, TimeUnit.MILLISECONDS` for better readability.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelineManagerImpl.java:
##########
@@ -568,28 +575,43 @@ public void testScrubPipelines() throws Exception {
.getInstance(ReplicationFactor.THREE),
Pipeline.PipelineState.CLOSED).contains(closedPipeline));
+ // Set the clock to "now". All pipelines were created before this.
+ testClock.set(Instant.now());
+
pipelineManager.scrubPipelines();
- // The allocatedPipeline should be scrubbed.
- Assert.assertFalse(pipelineManager
+ // The allocatedPipeline should not be scrubbed as the interval has not
+ // yet passed.
+ Assert.assertTrue(pipelineManager
.getPipelines(RatisReplicationConfig
.getInstance(ReplicationFactor.THREE),
Pipeline.PipelineState.ALLOCATED).contains(allocatedPipeline));
- // The closedPipeline should be scrubbed.
+ // The closedPipeline should be scrubbed, as they are scrubbed immediately
Assert.assertFalse(pipelineManager
.getPipelines(RatisReplicationConfig
.getInstance(ReplicationFactor.THREE),
Pipeline.PipelineState.CLOSED).contains(closedPipeline));
+ testClock.fastForward((60000));
+
+ pipelineManager.scrubPipelines();
+
+ // The allocatedPipeline should not be scrubbed as the interval has not
+ // yet passed.
+ Assert.assertFalse(pipelineManager
+ .getPipelines(RatisReplicationConfig
+ .getInstance(ReplicationFactor.THREE),
+ Pipeline.PipelineState.ALLOCATED).contains(allocatedPipeline));
Review Comment:
I think the comment is wrong, since we are checking that the pipeline is no longer present.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelineManagerImpl.java:
##########
@@ -568,28 +575,43 @@ public void testScrubPipelines() throws Exception {
.getInstance(ReplicationFactor.THREE),
Pipeline.PipelineState.CLOSED).contains(closedPipeline));
+ // Set the clock to "now". All pipelines were created before this.
+ testClock.set(Instant.now());
+
pipelineManager.scrubPipelines();
- // The allocatedPipeline should be scrubbed.
- Assert.assertFalse(pipelineManager
+ // The allocatedPipeline should not be scrubbed as the interval has not
+ // yet passed.
+ Assert.assertTrue(pipelineManager
.getPipelines(RatisReplicationConfig
.getInstance(ReplicationFactor.THREE),
Pipeline.PipelineState.ALLOCATED).contains(allocatedPipeline));
- // The closedPipeline should be scrubbed.
+ // The closedPipeline should be scrubbed, as they are scrubbed immediately
Assert.assertFalse(pipelineManager
.getPipelines(RatisReplicationConfig
.getInstance(ReplicationFactor.THREE),
Pipeline.PipelineState.CLOSED).contains(closedPipeline));
+ testClock.fastForward((60000));
+
+ pipelineManager.scrubPipelines();
+
+ // The allocatedPipeline should not be scrubbed as the interval has not
+ // yet passed.
+ Assert.assertFalse(pipelineManager
+ .getPipelines(RatisReplicationConfig
+ .getInstance(ReplicationFactor.THREE),
+ Pipeline.PipelineState.ALLOCATED).contains(allocatedPipeline));
+
pipelineManager.close();
}
@Test
public void testScrubPipelinesShouldFailOnFollower() throws Exception {
// No timeout for pipeline scrubber.
conf.setTimeDuration(
- OZONE_SCM_PIPELINE_ALLOCATED_TIMEOUT, -1,
+ OZONE_SCM_PIPELINE_ALLOCATED_TIMEOUT, 10000,
TimeUnit.MILLISECONDS);
Review Comment:
Here, too, I guess "no timeout" comment is no longer valid.
--
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