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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
