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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
