You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ronitsanan123 (via GitHub)" <gi...@apache.org> on 2023/10/30 17:32:13 UTC
[PR] Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
ronitsanan123 opened a new pull request, #5516:
URL: https://github.com/apache/ozone/pull/5516
## What changes were proposed in this pull request?
HDDS-8866:
Some SCM unit tests sleep() to "ensure" heartbeat is processed. In this PR it is improved by using TestClock.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8866
## How was this patch tested?
Added improvements to existing tests only
--
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
Re: [PR] Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5516:
URL: https://github.com/apache/ozone/pull/5516#discussion_r1382949475
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java:
##########
@@ -217,6 +219,7 @@ SCMNodeManager createNodeManager(OzoneConfiguration config)
@Test
public void testScmHeartbeat()
throws IOException, InterruptedException, AuthenticationException {
+ TestClock testClock = TestClock.newInstance();
Review Comment:
`SCMNodeManager` does not know about this `testClock`, its `systemClock` is always a real one. So SCM sees the actual time, even if the test fast-forwards its test clock.
We need to allow the test to pass its `TestClock` to `StorageContainerManager`. This can be achieved by the following:
1. add a `Clock` to `SCMConfigurator`
2. set `systemClock` in `StorageContainerManager` conditionally similar to how it takes other objects from the configurator (i.e. set from the configurator, if it has a clock, otherwise using the existing code)
https://github.com/apache/ozone/blob/08131b9addc719551a8e4d8c1846f8c8f11bd62c/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java#L637-L643
Example:
```
if (configurator.getClock() != null) {
systemClock = configurator.getClock();
} else {
// Use SystemClock when data is persisted
// and used again after system restarts.
systemClock = Clock.system(ZoneOffset.UTC);
}
```
3. Add a new version of `HddsTestUtils.getScm()` to allow passing a `Clock`:
https://github.com/apache/ozone/blob/08131b9addc719551a8e4d8c1846f8c8f11bd62c/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/HddsTestUtils.java#L617
--
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
Re: [PR] Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5516:
URL: https://github.com/apache/ozone/pull/5516#discussion_r1381559271
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -176,6 +178,7 @@ public SCMNodeManager(OzoneConfiguration conf,
String dnLimit = conf.get(ScmConfigKeys.OZONE_DATANODE_PIPELINE_LIMIT);
this.heavyNodeCriteria = dnLimit == null ? 0 : Integer.parseInt(dnLimit);
this.scmContext = scmContext;
+ this.clock = clock;
Review Comment:
There are other places where `Time.monotonicNow()` in `NodeStateManager`, `SCMNodeManager` and `DatanodeInfo`, I think we should replace those, too.
--
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
Re: [PR] HDDS-8866. Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5516:
URL: https://github.com/apache/ozone/pull/5516#issuecomment-1837978644
Thanks @ronitsanan for continuing work on this.
Checkstyle can be run locally very easily and quickly, so you don't need to wait for CI to report problems:
```
hadoop-ozone/dev-support/checks/checkstyle.sh
```
Also, please merge `master` into your branch as there is some conflict. Assuming `apache/ozone` remote repo is named `origin` locally:
```
git fetch --all
git merge origin/master
# edit conflicting files:
# hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java
# hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
# hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/scm/TestReconNodeManager.java
# hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/scm/TestReconPipelineManager.java
# check if it still compiles:
mvn -DskipTests -DskipShade clean package
# finish merge:
git add -u
git merge --continue
```
--
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
Re: [PR] Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5516:
URL: https://github.com/apache/ozone/pull/5516#discussion_r1381465766
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -176,6 +178,7 @@ public SCMNodeManager(OzoneConfiguration conf,
String dnLimit = conf.get(ScmConfigKeys.OZONE_DATANODE_PIPELINE_LIMIT);
this.heavyNodeCriteria = dnLimit == null ? 0 : Integer.parseInt(dnLimit);
this.scmContext = scmContext;
+ this.clock = clock;
Review Comment:
Where does `SCMNodeManager` use this clock?
--
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
Re: [PR] Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "ronitsanan123 (via GitHub)" <gi...@apache.org>.
ronitsanan123 commented on code in PR #5516:
URL: https://github.com/apache/ozone/pull/5516#discussion_r1380771009
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestContainerPlacement.java:
##########
@@ -89,6 +92,10 @@ public class TestContainerPlacement {
private OzoneConfiguration conf;
private PipelineManager pipelineManager;
private NodeManager nodeManager;
+ private final Clock clock;
+ public TestContainerPlacement() {
+ this.clock = Clock.system(ZoneId.systemDefault());
Review Comment:
@adoroszlai Made the changes.Can you please review it once
--
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
Re: [PR] HDDS-8866. Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5516:
URL: https://github.com/apache/ozone/pull/5516#issuecomment-1839265969
> `More than 7 parameters (found 8).`
> It should have 8 parameters only.Dont know why it is failing
The checkstyle rule allows only 7, but there were 8. Having too many parameters usually indicates the code should be refactored.
> Can you suggest me a change
You can add `@SuppressWarnings("parameternumber")` to the constructor. There is no simple way to reduce its parameter count now.
--
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
Re: [PR] HDDS-8866. Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5516:
URL: https://github.com/apache/ozone/pull/5516#issuecomment-1839371870
@ronitsanan123 Please add `@SuppressWarnings("parameternumber")` as is, do not replace `"parameternumber"` with 8 just because the constructor has 8 parameters.
Take a look at similar code:
https://github.com/apache/ozone/blob/a4bda5c6a2da921b53d02c622f1b9d14c190fb91/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java#L118-L124
Also, using more descriptive (but not excessively long) commit message for each commit would be helpful (for both you and reviewers).
--
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
Re: [PR] HDDS-8866. Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "ronitsanan123 (via GitHub)" <gi...@apache.org>.
ronitsanan123 commented on code in PR #5516:
URL: https://github.com/apache/ozone/pull/5516#discussion_r1413208520
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java:
##########
@@ -217,6 +219,7 @@ SCMNodeManager createNodeManager(OzoneConfiguration config)
@Test
public void testScmHeartbeat()
throws IOException, InterruptedException, AuthenticationException {
+ TestClock testClock = TestClock.newInstance();
Review Comment:
ack
--
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
Re: [PR] HDDS-8866. Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "ronitsanan123 (via GitHub)" <gi...@apache.org>.
ronitsanan123 commented on PR #5516:
URL: https://github.com/apache/ozone/pull/5516#issuecomment-1839242697
@adoroszlai Can you suggest me a change for this checkstyle failure `hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java
155: More than 7 parameters (found 8).`It should have 8 parameters only.Dont know why it is failing
--
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
Re: [PR] HDDS-8866. Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "ronitsanan123 (via GitHub)" <gi...@apache.org>.
ronitsanan123 commented on PR #5516:
URL: https://github.com/apache/ozone/pull/5516#issuecomment-1839376625
Ack
--
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
Re: [PR] Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5516:
URL: https://github.com/apache/ozone/pull/5516#discussion_r1382402670
##########
hadoop-hdds/server-scm/pom.xml:
##########
@@ -165,6 +165,11 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd">
<artifactId>junit-jupiter-params</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.ozone</groupId>
+ <artifactId>hdds-test-utils</artifactId>
+ <scope>compile</scope>
Review Comment:
`hdds-test-utils` is for tests, should not be added in `compile` scope.
This seems to be unnecessary, compiles fine without this.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java:
##########
@@ -154,6 +160,10 @@ public class TestSCMNodeManager {
private static final LayoutVersionProto CORRECT_LAYOUT_PROTO =
toLayoutVersionProto(MAX_LV, MAX_LV);
+// public TestSCMNodeManager() {
+// this.clock = Clock.system(ZoneId.systemDefault());
Review Comment:
It would be good to create as instance variable instead of in each test method. But should use `TestClock`, not `Clock.system()`.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/MockNodeManager.java:
##########
@@ -275,12 +278,15 @@ public List<DatanodeDetails> getNodes(NodeStatus status) {
@Override
public List<DatanodeDetails> getNodes(
HddsProtos.NodeOperationalState opState, HddsProtos.NodeState nodestate) {
+ Instant initialInstant = Instant.now();
+ ZoneId zoneId = ZoneId.systemDefault();
+ TestClock testClock = new TestClock(initialInstant, zoneId);
Review Comment:
It would be nice to avoid duplicating these. Can you please try using `TestClock.newInstance()`?
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java:
##########
@@ -136,6 +140,8 @@ public class TestSCMNodeManager {
private static final Logger LOG =
LoggerFactory.getLogger(TestSCMNodeManager.class);
+// @Mock
+// private final Clock clock;
Review Comment:
Please remove dead code.
--
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
Re: [PR] Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "ronitsanan123 (via GitHub)" <gi...@apache.org>.
ronitsanan123 commented on PR #5516:
URL: https://github.com/apache/ozone/pull/5516#issuecomment-1786462304
Thanks @adoroszlai ,Sure i will work on the requested changes.Also i have made a new commit to fix the the checkstyle failures
--
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
Re: [PR] Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "ronitsanan123 (via GitHub)" <gi...@apache.org>.
ronitsanan123 commented on code in PR #5516:
URL: https://github.com/apache/ozone/pull/5516#discussion_r1381478672
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -176,6 +178,7 @@ public SCMNodeManager(OzoneConfiguration conf,
String dnLimit = conf.get(ScmConfigKeys.OZONE_DATANODE_PIPELINE_LIMIT);
this.heavyNodeCriteria = dnLimit == null ? 0 : Integer.parseInt(dnLimit);
this.scmContext = scmContext;
+ this.clock = clock;
Review Comment:
It is Used by TestSCMNodeManager ..Cant find its use in SCMNodeManager
--
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
Re: [PR] Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5516:
URL: https://github.com/apache/ozone/pull/5516#discussion_r1381555357
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -176,6 +178,7 @@ public SCMNodeManager(OzoneConfiguration conf,
String dnLimit = conf.get(ScmConfigKeys.OZONE_DATANODE_PIPELINE_LIMIT);
this.heavyNodeCriteria = dnLimit == null ? 0 : Integer.parseInt(dnLimit);
this.scmContext = scmContext;
+ this.clock = clock;
Review Comment:
These are the places where the "last heartbeat" time is stored:
https://github.com/apache/ozone/blob/e4ff5fa1b9b6a8335e5e1d7a118c9c3d4b46281d/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeInfo.java#L75
https://github.com/apache/ozone/blob/e4ff5fa1b9b6a8335e5e1d7a118c9c3d4b46281d/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeInfo.java#L88-L90
They should use `clock.millis()` instead of `Time.monotonicNow()`. We need to pass `clock` to `NodeStateManager` here:
https://github.com/apache/ozone/blob/e4ff5fa1b9b6a8335e5e1d7a118c9c3d4b46281d/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java#L151-L152
--
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
Re: [PR] Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5516:
URL: https://github.com/apache/ozone/pull/5516#discussion_r1379778612
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestContainerPlacement.java:
##########
@@ -89,6 +92,10 @@ public class TestContainerPlacement {
private OzoneConfiguration conf;
private PipelineManager pipelineManager;
private NodeManager nodeManager;
+ private final Clock clock;
+ public TestContainerPlacement() {
+ this.clock = Clock.system(ZoneId.systemDefault());
Review Comment:
`private final Clock clock` should not be added to the test, rather to the code being tested (`SCMNodeManager`). It should be passed to it as a parameter of the constructor. Unit test should create and pass a `TestClock`. Other code should pass the system clock.
Also, `SCMNodeManager` needs to be changed to use this clock.
--
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
Re: [PR] Avoid sleep in SCM tests waiting for heartbeat processing [ozone]
Posted by "ronitsanan123 (via GitHub)" <gi...@apache.org>.
ronitsanan123 commented on code in PR #5516:
URL: https://github.com/apache/ozone/pull/5516#discussion_r1382371152
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -176,6 +178,7 @@ public SCMNodeManager(OzoneConfiguration conf,
String dnLimit = conf.get(ScmConfigKeys.OZONE_DATANODE_PIPELINE_LIMIT);
this.heavyNodeCriteria = dnLimit == null ? 0 : Integer.parseInt(dnLimit);
this.scmContext = scmContext;
+ this.clock = clock;
Review Comment:
Ack
--
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