HDDS-8082. Check for unnecessary newlines in config defaults by cychiu8 · Pull Request #10840 · apache/ozone

cychiu8 · 2026-07-22T11:16:21Z

What changes were proposed in this pull request?

HDDS-8046 removed some unnecessary newlines embedded in default config values in ozone-default.xml. Ayush Saxena had a great suggestion for avoiding these in the future:

I thought of adding a test, which checks for all the configs - set of configs added into the exception list and warns like don't add such breaks unnecessarily, if it is important and legitimate then add it into the exception list, that way even the reviewer would be aware that this practice isn't encouraged.

What is the link to the Apache JIRA

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

How was this patch tested?

Test locally with the following command:

mvn -pl :ozone-integration-test test \
-Dtest='TestOzoneConfigurationFields#testXmlValuesHaveNoEmbeddedNewlines' \
-DskipShade -DskipRecon

Testing three scenarios:

Modify a value to include a newline: should fail
Add a test property with an intentional newline in its value: should pass
Validate the current ozone-default.xml: should pass

Detailed results for case 1 (Modify a value to include a newline: should fail)

[INFO] Running org.apache.hadoop.ozone.TestOzoneConfigurationFields
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.236 s <<< FAILURE! -- in org.apache.hadoop.ozone.TestOzoneConfigurationFields
[ERROR] org.apache.hadoop.ozone.TestOzoneConfigurationFields.testXmlValuesHaveNoEmbeddedNewlines -- Time elapsed: 0.163 s <<< FAILURE!
org.opentest4j.AssertionFailedError: These properties in ozone-default.xml have an embedded line break in their <value>, which corrupts the runtime string:  [ozone.client.key.latest.version.location] Put the value on a single line. If the newline is genuinely required, add the property to xmlPropsAllowedToContainNewline (in initializeMemberVariables) with a reason + Jira. See HDDS-8082. ==> expected: <true> but was: <false>
        at org.apache.hadoop.conf.ConfigurationFieldsTests.testXmlValuesHaveNoEmbeddedNewlines(ConfigurationFieldsTests.java:700)
        at java.base/java.lang.reflect.Method.invoke(Method.java:580)
        at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:387)
        at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1312)
        at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1843)
        at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1808)
        at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:188)

[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   TestOzoneConfigurationFields>ConfigurationFieldsTests.testXmlValuesHaveNoEmbeddedNewlines:700 These properties in ozone-default.xml have an embedded line break in their <value>, which corrupts the runtime string:  [ozone.client.key.latest.version.location] Put the value on a single line. If the newline is genuinely required, add the property to xmlPropsAllowedToContainNewline (in initializeMemberVariables) with a reason + Jira. See HDDS-8082. ==> expected: <true> but was: <false>                                                                                                                                                                                                             
[INFO] 
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  11.897 s
[INFO] Finished at: 2026-07-22T17:11:09+09:00

Detailed results for case 2(Add a test property with an intentional newline in its value: should pass)
log:

2026-07-22 19:42:58,104 [main] INFO  conf.ConfigurationFieldsTests (ConfigurationFieldsTests.java:testXmlValuesHaveNoEmbeddedNewlines(696)) - XML Property: ozone.client.key.latest.version.location AllowedToContainNewline

[INFO] Running org.apache.hadoop.ozone.TestOzoneConfigurationFields
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.191 s -- in org.apache.hadoop.ozone.TestOzoneConfigurationFields
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  10.258 s
[INFO] Finished at: 2026-07-22T19:49:01+09:00
[INFO] ------------------------------------------------------------------------

Detailed results for case 3(Validate the current ozone-default.xml: should pass)

[INFO] Running org.apache.hadoop.ozone.TestOzoneConfigurationFields
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.158 s -- in org.apache.hadoop.ozone.TestOzoneConfigurationFields
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------

…eldsTests

cychiu8 · 2026-07-22T12:47:32Z

@adoroszlai Regarding HDDS-8082 you created, here is the PR. Happy to have your feedback whenever you get a chance.

chihsuan

Thanks for picking this up! I left one question about properties filtered by xmlPropsToSkipCompare and xmlPrefixToSkipCompare, as they may unintentionally bypass the new check. The other comments are optional readability suggestions.

Overall, the approach looks good to me. Thanks!

chihsuan · 2026-07-22T12:53:51Z

+  @Test
+  public void testXmlValuesHaveNoEmbeddedNewlines() {
+    Set<String> xmlValuesWithNewlines = new HashSet<>();
+    for (Map.Entry<String, String> entry : xmlKeyValueMap.entrySet()) {


Would it make sense to check newlines before applying xmlPropsToSkipCompare and xmlPrefixToSkipCompare? therwise, properties excluded from field comparison also skip this check, even if they are not listed in xmlPropsAllowedToContainNewline.

Thank you for reviewing. You mentioned a great point that I should decouple the newline check, which should not depend on the excluded properties.

chihsuan · 2026-07-22T12:57:00Z


+    assertTrue(xmlValuesWithNewlines.isEmpty(),
+        "These properties in " + xmlFilename + " have an embedded line break in "
+            + "their <value>, which corrupts the runtime string:  " + xmlValuesWithNewlines


nit: There is an extra space after runtime string:.

Nice catch. Thank you!

chihsuan · 2026-07-22T12:58:09Z

+   */
+  @Test
+  public void testXmlValuesHaveNoEmbeddedNewlines() {
+    Set<String> xmlValuesWithNewlines = new HashSet<>();


nit: Would a TreeSet be useful here to keep the failure output deterministic? The sibling checks in this class also use it.

Thank you for the feedback. It makes sense! I have addressed it.

chihsuan

Thanks @cychiu8 LGTM!

adoroszlai

Thanks @cychiu8 for the patch.

adoroszlai · 2026-07-25T15:46:06Z

+      }
+    }

+    assertTrue(xmlValuesWithNewlines.isEmpty(),


nit: please use assertThat(xmlValuesWithNewlines).isEmpty(), see HDDS-9951

Thank you for the review and feedback! I've addressed it.

ayushtkn

LGTM

adoroszlai

Thanks again @cychiu8 for the patch. On another look I found one more issue.

adoroszlai · 2026-07-27T15:10:10Z

+    Configuration conf = new Configuration(false);
+    conf.setAllowNullValueProperties(true);
+    conf.addResource(xmlFilename);


We need to use OzoneConfiguration to handle generated configuration files, too. Example violation to reproduce the problem:

--- hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/OzoneClientConfig.java +++ hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/OzoneClientConfig.java @@ -41,7 +41,7 @@ public class OzoneClientConfig { defaultValue = "16MB", type = ConfigType.SIZE, description = "Size which determines at what buffer position a partial " - + "flush will be initiated during write. It should be a multiple of" + + "flush will be initiated during write.\n\nIt should be a multiple of" + " ozone.client.stream.buffer.size", tags = ConfigTag.CLIENT) private long streamBufferFlushSize = 16 * 1024 * 1024;

Suggestion:

Configuration conf = new OzoneConfiguration(); conf.setAllowNullValueProperties(true);

Needs new import as well.

Probably worth tweaking the assertion failure message to omit mention of ozone-default.xml, since that's not the only file where the property may live.

I also suggest moving the test case to TestOzoneConfigurationFields, because ConfigurationFieldsTests is mostly supposed to be a copy of Hadoop's TestConfigurationFieldsBase.

Thanks @adoroszlai again.

I've switched the test to OzoneConfiguration, moved it to TestOzoneConfigurationFields, and dropped the ozone-default.xml reference from the failure message.

It now covers the generated <module>-default.xml files too.

Loading via OzoneConfiguration also brings in Hadoop's core-default.xml, which legitimately ships four multi-line values:

fs.s3a.aws.credentials.provider

hadoop.security.sensitive-config-keys

hadoop.system.tags

hadoop.tags.system

I've added those to xmlPropsAllowedToContainNewline with a comment.

HDDS-8082. Add testXmlValuesHaveNoEmbeddedNewlines to ConfigurationFi…

c6557d2

…eldsTests

chihsuan reviewed Jul 22, 2026

View reviewed changes

Fix: Read raw XML directly in testXmlValuesHaveNoEmbeddedNewlines

51c3a8b

chihsuan approved these changes Jul 23, 2026

View reviewed changes

adoroszlai reviewed Jul 25, 2026

View reviewed changes

fix: use assertThat instead of assertTrue

3f72dc8

adoroszlai requested a review from ayushtkn July 26, 2026 20:30

ayushtkn approved these changes Jul 27, 2026

View reviewed changes

adoroszlai reviewed Jul 27, 2026

View reviewed changes

fix: include generated config files for embedded newlines

c8ad9c4

Uh oh!

Conversation

cychiu8 commented Jul 22, 2026

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

cychiu8 commented Jul 22, 2026

Uh oh!

chihsuan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chihsuan left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayushtkn left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cychiu8 Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cychiu8 Jul 29, 2026 •

edited

Loading