umamaheswararao commented on code in PR #5084:
URL: https://github.com/apache/ozone/pull/5084#discussion_r1270308822
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/WritableECContainerProvider.java:
##########
@@ -159,13 +160,21 @@ public ContainerInfo getContainer(final long size,
// If we get here, all the pipelines we tried were no good. So try to
// allocate a new one.
try {
- synchronized (this) {
- if (openPipelineCount < maximumPipelines) {
+ if (openPipelineCount >= maximumPipelines) {
+ final int nodeCount = nodeManager.getNodeCount(null, null);
+ if (nodeCount > maximumPipelines) {
+ LOG.debug("Increasing pipeline limit {} -> {} for final attempt",
+ maximumPipelines, nodeCount);
Review Comment:
This seems like one time log and a valuable setting change happening. Is it
make sense to bump this log level?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/WritableECContainerProvider.java:
##########
@@ -159,13 +160,21 @@ public ContainerInfo getContainer(final long size,
// If we get here, all the pipelines we tried were no good. So try to
// allocate a new one.
try {
- synchronized (this) {
- if (openPipelineCount < maximumPipelines) {
+ if (openPipelineCount >= maximumPipelines) {
+ final int nodeCount = nodeManager.getNodeCount(null, null);
+ if (nodeCount > maximumPipelines) {
+ LOG.debug("Increasing pipeline limit {} -> {} for final attempt",
+ maximumPipelines, nodeCount);
+ maximumPipelines = nodeCount;
Review Comment:
Still users may need to tune this value based on their cluster and
ECReplication config. Let's using 10:4 will different number openpipeline per
node vs using 3:2 will have different number of pipelines per node. Because
number of node involve in a pipeline changes. It is hard to auto tune that.
(Vague thought)But later we may want to think like 80% nodes max pipeline count
limit or so. if we get the info like each DN openpipelines counts, then if 80%(
just an argitrary number) nodes in the cluster already have that max open
pipelines, then we are at cap of openpipelines limit.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/MockNodeManager.java:
##########
@@ -309,6 +309,10 @@ public List<DatanodeDetails> getNodes(
return deadNodes;
}
+ if (nodestate == null) {
Review Comment:
oh it's because your passing null when calling getNodeCount?
That means null makes it as any state.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/WritableECContainerProvider.java:
##########
@@ -159,13 +160,21 @@ public ContainerInfo getContainer(final long size,
// If we get here, all the pipelines we tried were no good. So try to
// allocate a new one.
try {
- synchronized (this) {
- if (openPipelineCount < maximumPipelines) {
+ if (openPipelineCount >= maximumPipelines) {
Review Comment:
Should this exclude DECOMMISSIONED state nodes?
What if more nodes take to offline mode? then this count puts the pressure
on cluster with more openpipelines? ( May be rare case as people may not take
many nodes at a time though )
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/MockNodeManager.java:
##########
@@ -309,6 +309,10 @@ public List<DatanodeDetails> getNodes(
return deadNodes;
}
+ if (nodestate == null) {
Review Comment:
instead of passing null, getAllNodes().size() also works?
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/MockNodeManager.java:
##########
@@ -309,6 +309,10 @@ public List<DatanodeDetails> getNodes(
return deadNodes;
}
+ if (nodestate == null) {
Review Comment:
What is the case nodestate is null?
--
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]
