HDDS-14369. RatisPipelineProvider does not honor OZONE_DATANODE_PIPELINE_LIMIT_DEFAULT #9609

Russole · 2026-01-08T22:58:57Z

What changes were proposed in this pull request?

Fix inconsistent default handling for OZONE_DATANODE_PIPELINE_LIMIT
Use conf.getInt(..., DEFAULT) instead of falling back to 0 when the config is unset
Ensure the behavior matches OZONE_DATANODE_PIPELINE_LIMIT_DEFAULT (default = 2)

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-14369

How was this patch tested?

All CI checks passed.
https://github.com/Russole/ozone/actions/runs/20823196572

…INE_LIMIT_DEFAULT

Gargi-jais11

Thanks @Russole for working on the patch.
There are two more classes where OZONE_DATANODE_PIPELINE_LIMIT_DEFAULT value should be used instead of 0.
SCMNodeManager : https://github.com/Gargi-jais11/ozone/blob/64bb019407bc001fbe2c6339141908c3e7d59b8f/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java#L198-L199
PipelinePlacementPolicy : https://github.com/Gargi-jais11/ozone/blob/64bb019407bc001fbe2c6339141908c3e7d59b8f/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java#L79-L80

Each layer needs the correct default value because they all independently read the configuration and use it in different ways. Fixing only one would leave inconsistencies in the others, causing incorrect behavior.

Gargi-jais11 · 2026-01-09T05:41:49Z

It is also good if you add test cases for this part in TestPipelinePlacementPolicy, TestRatisPipelineProvider and TestSCMNodeManager.
1. RatisPipelineProvider

Test flow:
- Creates config without setting pipeline limit
- Creates pipelines up to expected limit (6 pipelines with 10 nodes)
- Verifies 7th pipeline creation throws SCMException
- Validates error message mentions default limit of 2

Coverage:
- Default value (2) used instead of 0 
- Pipeline limit enforcement works 
- Error messages accurate

2. PipelinePlacementPolicy

Test flow:
- Creates policy with config without limit set
- Adds exactly 2 pipelines to test node (default limit)
- Verifies node is filtered out when choosing nodes for new pipeline

3. SCMNodeManager

Test flow:
- Creates node manager with config without limit set
- Registers datanode with healthy volumes
- Calls pipelineLimit() and verifies returns 2

Russole · 2026-01-10T01:32:58Z

Thanks @Gargi-jais11 for the reviews. I’ve addressed the comments and updated the patch accordingly.

Gargi-jais11

Thank @Russole for updating the patch. Please fine some more comments inlined.

Gargi-jais11 · 2026-01-12T04:35:14Z

hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java

+      int limit = nodeManager.pipelineLimit(dn);
+      assertEquals(ScmConfigKeys.OZONE_DATANODE_PIPELINE_LIMIT_DEFAULT, limit);
+      assertEquals(2, limit);


This assertion is redundant. The first assertion itself is enough to check that it picks the default value.

Gargi-jais11 · 2026-01-12T04:36:23Z

hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java

+      // Registers datanode with healthy volumes
+      DatanodeDetails dn = registerWithCapacity(nodeManager);
+
+      // Calls pipelineLimit() and verifies returns 2


Suggested change

// Calls pipelineLimit() and verifies returns 2

// Calls pipelineLimit() and verifies returns default value

Gargi-jais11 · 2026-01-12T04:38:41Z

hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java

+   * Test that pipelineLimit() uses the default value when the config is not set.
+   */
+  @Test
+  public void testPipelineLimitDefaultIsTwoWhenUnset()


rename it as something more generic:testUsesDefaultPipelineLimitWhenUnset()

Gargi-jais11 · 2026-01-12T04:42:51Z

@szetszwo Please review this patch.

szetszwo

@Russole , thanks for working on this! The change looks good. Just have some comments on the existing names.

szetszwo · 2026-01-12T16:25:01Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java

            ScmConfigKeys.OZONE_SCM_PIPELINE_PER_METADATA_VOLUME_DEFAULT);
-    String dnLimit = conf.get(ScmConfigKeys.OZONE_DATANODE_PIPELINE_LIMIT);
-    this.heavyNodeCriteria = dnLimit == null ? 0 : Integer.parseInt(dnLimit);
+    this.heavyNodeCriteria = conf.getInt(


The existing field name heavyNodeCriteria is quite creative but unclear what does it mean. Could you rename it to datanodePipelineLimit?

szetszwo · 2026-01-12T17:27:07Z

...hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/RatisPipelineProvider.java

-    String dnLimit = conf.get(ScmConfigKeys.OZONE_DATANODE_PIPELINE_LIMIT);
-    this.maxPipelinePerDatanode = dnLimit == null ? 0 :
-        Integer.parseInt(dnLimit);
+    this.maxPipelinePerDatanode = conf.getInt(


Let's also rename it to datanodePipelineLimit. So it is easier to tell that they are the same thing.

jojochuang · 2026-01-12T17:30:31Z

This issue is related to #9580

…neLimit

Russole · 2026-01-12T17:36:13Z

Thanks @szetszwo and @jojochuang for the review. I’ve updated the implementation based on the comments.

szetszwo · 2026-01-12T17:37:10Z

...ds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java

    this.stateManager = stateManager;
-    String dnLimit = conf.get(ScmConfigKeys.OZONE_DATANODE_PIPELINE_LIMIT);
-    this.heavyNodeCriteria = dnLimit == null ? 0 : Integer.parseInt(dnLimit);
+    this.heavyNodeCriteria = conf.getInt(


@Russole , there is one more field to rename.

Thanks for the reminder.

szetszwo

+1 the change looks good

Gargi-jais11

LGTM

HDDS-14369. RatisPipelineProvider does not honor OZONE_DATANODE_PIPEL…

ac97639

…INE_LIMIT_DEFAULT

Gargi-jais11 suggested changes Jan 9, 2026

View reviewed changes

Ensure default pipeline limit is applied and covered by tests

6d23d87

Russole requested a review from Gargi-jais11 January 10, 2026 03:09

Gargi-jais11 reviewed Jan 12, 2026

View reviewed changes

Merge branch 'master' into HDDS-14369

227ea7d

jojochuang requested a review from szetszwo January 12, 2026 17:27

Update patch based on review comments

5410c56

szetszwo reviewed Jan 12, 2026

View reviewed changes

Rename maxPipelinePerDatanode and heavyNodeCriteria to datanodePipeli…

9c03621

…neLimit

szetszwo reviewed Jan 12, 2026

View reviewed changes

Complete renaming to datanodePipelineLimit

ac32ac4

Russole requested review from Gargi-jais11 and szetszwo January 12, 2026 17:40

Merge branch 'master' into HDDS-14369

b1df2ee

szetszwo approved these changes Jan 12, 2026

View reviewed changes

Gargi-jais11 approved these changes Jan 13, 2026

View reviewed changes

szetszwo merged commit ef4bb7f into apache:master Jan 13, 2026
44 checks passed

	// Calls pipelineLimit() and verifies returns 2
	// Calls pipelineLimit() and verifies returns default value

HDDS-14369. RatisPipelineProvider does not honor OZONE_DATANODE_PIPELINE_LIMIT_DEFAULT #9609

HDDS-14369. RatisPipelineProvider does not honor OZONE_DATANODE_PIPELINE_LIMIT_DEFAULT #9609

Uh oh!

Conversation

Russole commented Jan 8, 2026

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Gargi-jais11 left a comment

Choose a reason for hiding this comment

Uh oh!

Gargi-jais11 commented Jan 9, 2026

Uh oh!

Russole commented Jan 10, 2026

Uh oh!

Gargi-jais11 left a comment

Choose a reason for hiding this comment

Uh oh!

Gargi-jais11 Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Gargi-jais11 Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Gargi-jais11 Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Gargi-jais11 commented Jan 12, 2026

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

szetszwo Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

szetszwo Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

jojochuang commented Jan 12, 2026

Uh oh!

Russole commented Jan 12, 2026

Uh oh!

szetszwo Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Russole Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Gargi-jais11 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants