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 2021/07/20 18:25:43 UTC
[GitHub] [ozone] adoroszlai opened a new pull request #2445: HDDS-5468. Avoid long sleep in TestPeriodicVolumeChecker
adoroszlai opened a new pull request #2445:
URL: https://github.com/apache/ozone/pull/2445
## What changes were proposed in this pull request?
Eliminate the need for costly sleep in `TestPeriodicVolumeChecker`:
```
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 132.95 s - in org.apache.hadoop.ozone.container.common.volume.TestPeriodicVolumeChecker
```
https://github.com/apache/ozone/runs/3112345557#step:4:923
by:
* manually calling `checkAllVolumeSets` instead of waiting for the real executor to schedule it,
* and using `FakeTimer` to "advance" time between the calls.
https://issues.apache.org/jira/browse/HDDS-5468
## How was this patch tested?
```
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.276 s - in org.apache.hadoop.ozone.container.common.volume.TestPeriodicVolumeChecker
```
https://github.com/adoroszlai/hadoop-ozone/runs/3115013489#step:4:1100
--
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] guihecheng commented on pull request #2445: HDDS-5468. Avoid long sleep in TestPeriodicVolumeChecker
Posted by GitBox <gi...@apache.org>.
guihecheng commented on pull request #2445:
URL: https://github.com/apache/ozone/pull/2445#issuecomment-883841898
Thanks @adoroszlai , the fake timer way is great, I also happen to learn this idea from @sodonnel 's patch https://github.com/apache/ozone/pull/2429, and plan to do a similar fix for this test.
The patch LGTM with some inline comments on the old code comments.
--
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] guihecheng commented on a change in pull request #2445: HDDS-5468. Avoid long sleep in TestPeriodicVolumeChecker
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #2445:
URL: https://github.com/apache/ozone/pull/2445#discussion_r673618104
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestPeriodicVolumeChecker.java
##########
@@ -79,40 +81,54 @@ public void testPeriodicVolumeChecker() throws Exception {
DatanodeConfiguration dnConf =
conf.getObject(DatanodeConfiguration.class);
- dnConf.setDiskCheckMinGap(Duration.ofMinutes(2));
- dnConf.setPeriodicDiskCheckIntervalMinutes(1);
- conf.setFromObject(dnConf);
-
- DatanodeDetails datanodeDetails =
- ContainerTestUtils.createDatanodeDetails();
- OzoneContainer ozoneContainer =
- ContainerTestUtils.getOzoneContainer(datanodeDetails, conf);
- MutableVolumeSet dataVolumeSet = ozoneContainer.getVolumeSet();
-
- StorageVolumeChecker volumeChecker = dataVolumeSet.getVolumeChecker();
- volumeChecker.setDelegateChecker(
- new TestStorageVolumeChecker.DummyChecker());
-
- // 1 for volumeSet and 1 for metadataVolumeSet
- // in MutableVolumeSet constructor
- Assert.assertEquals(2, volumeChecker.getNumAllVolumeChecks());
- Assert.assertEquals(0, volumeChecker.getNumAllVolumeSetsChecks());
-
- // wait for periodic disk checker start
- Thread.sleep((60 + 5) * 1000);
-
- // first round
- // 2 for volumeSet and 2 for metadataVolumeSet
- Assert.assertEquals(4, volumeChecker.getNumAllVolumeChecks());
- Assert.assertEquals(1, volumeChecker.getNumAllVolumeSetsChecks());
- Assert.assertEquals(0, volumeChecker.getNumSkippedChecks());
-
- // wait for periodic disk checker next round
- Thread.sleep((60 + 5) * 1000);
-
- // skipped next round
- Assert.assertEquals(4, volumeChecker.getNumAllVolumeChecks());
- Assert.assertEquals(1, volumeChecker.getNumAllVolumeSetsChecks());
- Assert.assertEquals(1, volumeChecker.getNumSkippedChecks());
+ Duration gap = dnConf.getDiskCheckMinGap();
+ Duration interval = Duration.ofMinutes(
+ dnConf.getPeriodicDiskCheckIntervalMinutes());
+
+ FakeTimer timer = new FakeTimer();
+
+ StorageVolumeChecker volumeChecker = new StorageVolumeChecker(conf, timer);
+
+ try {
+ volumeChecker.registerVolumeSet(new ImmutableVolumeSet(makeVolumes(
+ 2, HEALTHY)));
+ volumeChecker.registerVolumeSet(new ImmutableVolumeSet(makeVolumes(
+ 1, HEALTHY)));
+ volumeChecker.setDelegateChecker(
+ new TestStorageVolumeChecker.DummyChecker());
+
+ // 1 for volumeSet and 1 for metadataVolumeSet
+ // in MutableVolumeSet constructor
+ Assert.assertEquals(0, volumeChecker.getNumAllVolumeChecks());
+ Assert.assertEquals(0, volumeChecker.getNumAllVolumeSetsChecks());
+
+ // first round
+ timer.advance(gap.toMillis() / 3);
+ volumeChecker.checkAllVolumeSets();
+
+ // 2 for volumeSet and 2 for metadataVolumeSet
Review comment:
As mentioned above, we'll get 1 for each test VolumeSet, so old comments could be updated.
--
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 pull request #2445: HDDS-5468. Avoid long sleep in TestPeriodicVolumeChecker
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2445:
URL: https://github.com/apache/ozone/pull/2445#issuecomment-884980894
Thanks @guihecheng for the review, @bshashikant for reviewing and committing it.
--
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 change in pull request #2445: HDDS-5468. Avoid long sleep in TestPeriodicVolumeChecker
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2445:
URL: https://github.com/apache/ozone/pull/2445#discussion_r673688886
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestPeriodicVolumeChecker.java
##########
@@ -79,40 +81,54 @@ public void testPeriodicVolumeChecker() throws Exception {
DatanodeConfiguration dnConf =
conf.getObject(DatanodeConfiguration.class);
- dnConf.setDiskCheckMinGap(Duration.ofMinutes(2));
- dnConf.setPeriodicDiskCheckIntervalMinutes(1);
- conf.setFromObject(dnConf);
-
- DatanodeDetails datanodeDetails =
- ContainerTestUtils.createDatanodeDetails();
- OzoneContainer ozoneContainer =
- ContainerTestUtils.getOzoneContainer(datanodeDetails, conf);
- MutableVolumeSet dataVolumeSet = ozoneContainer.getVolumeSet();
-
- StorageVolumeChecker volumeChecker = dataVolumeSet.getVolumeChecker();
- volumeChecker.setDelegateChecker(
- new TestStorageVolumeChecker.DummyChecker());
-
- // 1 for volumeSet and 1 for metadataVolumeSet
- // in MutableVolumeSet constructor
- Assert.assertEquals(2, volumeChecker.getNumAllVolumeChecks());
- Assert.assertEquals(0, volumeChecker.getNumAllVolumeSetsChecks());
-
- // wait for periodic disk checker start
- Thread.sleep((60 + 5) * 1000);
-
- // first round
- // 2 for volumeSet and 2 for metadataVolumeSet
- Assert.assertEquals(4, volumeChecker.getNumAllVolumeChecks());
- Assert.assertEquals(1, volumeChecker.getNumAllVolumeSetsChecks());
- Assert.assertEquals(0, volumeChecker.getNumSkippedChecks());
-
- // wait for periodic disk checker next round
- Thread.sleep((60 + 5) * 1000);
-
- // skipped next round
- Assert.assertEquals(4, volumeChecker.getNumAllVolumeChecks());
- Assert.assertEquals(1, volumeChecker.getNumAllVolumeSetsChecks());
- Assert.assertEquals(1, volumeChecker.getNumSkippedChecks());
+ Duration gap = dnConf.getDiskCheckMinGap();
+ Duration interval = Duration.ofMinutes(
+ dnConf.getPeriodicDiskCheckIntervalMinutes());
+
+ FakeTimer timer = new FakeTimer();
+
+ StorageVolumeChecker volumeChecker = new StorageVolumeChecker(conf, timer);
+
+ try {
+ volumeChecker.registerVolumeSet(new ImmutableVolumeSet(makeVolumes(
+ 2, HEALTHY)));
+ volumeChecker.registerVolumeSet(new ImmutableVolumeSet(makeVolumes(
+ 1, HEALTHY)));
+ volumeChecker.setDelegateChecker(
+ new TestStorageVolumeChecker.DummyChecker());
+
+ // 1 for volumeSet and 1 for metadataVolumeSet
Review comment:
Thanks for reminding me. I updated these in a previous attempt, but then started over from scratch. Now removed.
--
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] bshashikant merged pull request #2445: HDDS-5468. Avoid long sleep in TestPeriodicVolumeChecker
Posted by GitBox <gi...@apache.org>.
bshashikant merged pull request #2445:
URL: https://github.com/apache/ozone/pull/2445
--
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] guihecheng commented on a change in pull request #2445: HDDS-5468. Avoid long sleep in TestPeriodicVolumeChecker
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #2445:
URL: https://github.com/apache/ozone/pull/2445#discussion_r673617664
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestPeriodicVolumeChecker.java
##########
@@ -79,40 +81,54 @@ public void testPeriodicVolumeChecker() throws Exception {
DatanodeConfiguration dnConf =
conf.getObject(DatanodeConfiguration.class);
- dnConf.setDiskCheckMinGap(Duration.ofMinutes(2));
- dnConf.setPeriodicDiskCheckIntervalMinutes(1);
- conf.setFromObject(dnConf);
-
- DatanodeDetails datanodeDetails =
- ContainerTestUtils.createDatanodeDetails();
- OzoneContainer ozoneContainer =
- ContainerTestUtils.getOzoneContainer(datanodeDetails, conf);
- MutableVolumeSet dataVolumeSet = ozoneContainer.getVolumeSet();
-
- StorageVolumeChecker volumeChecker = dataVolumeSet.getVolumeChecker();
- volumeChecker.setDelegateChecker(
- new TestStorageVolumeChecker.DummyChecker());
-
- // 1 for volumeSet and 1 for metadataVolumeSet
- // in MutableVolumeSet constructor
- Assert.assertEquals(2, volumeChecker.getNumAllVolumeChecks());
- Assert.assertEquals(0, volumeChecker.getNumAllVolumeSetsChecks());
-
- // wait for periodic disk checker start
- Thread.sleep((60 + 5) * 1000);
-
- // first round
- // 2 for volumeSet and 2 for metadataVolumeSet
- Assert.assertEquals(4, volumeChecker.getNumAllVolumeChecks());
- Assert.assertEquals(1, volumeChecker.getNumAllVolumeSetsChecks());
- Assert.assertEquals(0, volumeChecker.getNumSkippedChecks());
-
- // wait for periodic disk checker next round
- Thread.sleep((60 + 5) * 1000);
-
- // skipped next round
- Assert.assertEquals(4, volumeChecker.getNumAllVolumeChecks());
- Assert.assertEquals(1, volumeChecker.getNumAllVolumeSetsChecks());
- Assert.assertEquals(1, volumeChecker.getNumSkippedChecks());
+ Duration gap = dnConf.getDiskCheckMinGap();
+ Duration interval = Duration.ofMinutes(
+ dnConf.getPeriodicDiskCheckIntervalMinutes());
+
+ FakeTimer timer = new FakeTimer();
+
+ StorageVolumeChecker volumeChecker = new StorageVolumeChecker(conf, timer);
+
+ try {
+ volumeChecker.registerVolumeSet(new ImmutableVolumeSet(makeVolumes(
+ 2, HEALTHY)));
+ volumeChecker.registerVolumeSet(new ImmutableVolumeSet(makeVolumes(
+ 1, HEALTHY)));
+ volumeChecker.setDelegateChecker(
+ new TestStorageVolumeChecker.DummyChecker());
+
+ // 1 for volumeSet and 1 for metadataVolumeSet
Review comment:
We could remove the old comments here, since we are using ImmutableVolumeSet for test, and the checkAllVolumes call in MutableVolumeSet constructor will not be counted.
--
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] guihecheng edited a comment on pull request #2445: HDDS-5468. Avoid long sleep in TestPeriodicVolumeChecker
Posted by GitBox <gi...@apache.org>.
guihecheng edited a comment on pull request #2445:
URL: https://github.com/apache/ozone/pull/2445#issuecomment-883841898
Thanks @adoroszlai , the fake timer way is great, I also happen to learn this idea from @sodonnel 's patch https://github.com/apache/ozone/pull/2425, and plan to do a similar fix for this test.
The patch LGTM with some inline comments on the old code comments.
--
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