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 2020/07/27 08:40:55 UTC
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request #1257: HDDS-3970. Enabling TestStorageContainerManager with all failures add…
adoroszlai commented on a change in pull request #1257:
URL: https://github.com/apache/hadoop-ozone/pull/1257#discussion_r460726639
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestStorageContainerManager.java
##########
@@ -525,7 +523,7 @@ public void testScmInfo() throws Exception {
/**
* Test datanode heartbeat well processed with a 4-layer network topology.
*/
- @Test(timeout = 60000)
+ @Test(timeout = 180000)
public void testScmProcessDatanodeHeartbeat() throws Exception {
Review comment:
Sleep lengths in `testCloseContainerCommandOnRestart` are being increased. Why do we need to increase timeout on another test method?
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestStorageContainerManager.java
##########
@@ -604,8 +602,13 @@ public void testCloseContainerCommandOnRestart() throws Exception {
// Stop processing HB
scm.getDatanodeProtocolServer().stop();
- scm.getContainerManager().updateContainerState(selectedContainer
- .containerID(), HddsProtos.LifeCycleEvent.FINALIZE);
+ LoggerFactory.getLogger(TestStorageContainerManager.class).info(
+ "Current Container State is" + selectedContainer.getState());
Review comment:
Please use placeholder in log message instead of `+` to avoid new code warning (sonar).
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestStorageContainerManager.java
##########
@@ -604,8 +602,13 @@ public void testCloseContainerCommandOnRestart() throws Exception {
// Stop processing HB
scm.getDatanodeProtocolServer().stop();
- scm.getContainerManager().updateContainerState(selectedContainer
- .containerID(), HddsProtos.LifeCycleEvent.FINALIZE);
+ LoggerFactory.getLogger(TestStorageContainerManager.class).info(
Review comment:
Why not `LOG.info(`?
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestStorageContainerManager.java
##########
@@ -604,8 +602,13 @@ public void testCloseContainerCommandOnRestart() throws Exception {
// Stop processing HB
scm.getDatanodeProtocolServer().stop();
- scm.getContainerManager().updateContainerState(selectedContainer
- .containerID(), HddsProtos.LifeCycleEvent.FINALIZE);
+ LoggerFactory.getLogger(TestStorageContainerManager.class).info(
+ "Current Container State is" + selectedContainer.getState());
+ if (selectedContainer.getState() == HddsProtos.LifeCycleState.OPEN) {
+ scm.getContainerManager().updateContainerState(selectedContainer
+ .containerID(), HddsProtos.LifeCycleEvent.FINALIZE);
Review comment:
Do you know what other thread changed container state to CLOSING? Don't we still have a race condition here: in the time window between `getState` and `updateContainerState` that other thread could still change state of the container?
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestStorageContainerManager.java
##########
@@ -593,7 +591,7 @@ public void testCloseContainerCommandOnRestart() throws Exception {
new TestStorageContainerManagerHelper(cluster, conf);
helper.createKeys(10, 4096);
- Thread.sleep(5000);
+ Thread.sleep(10000);
Review comment:
Repeated check (eg. `GenericTestUtils.waitFor`) should be preferred over fixed length sleep:
1. allow quicker test execution if possible
2. explicit condition documents what the test is waiting for
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org