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!

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

HDDS-8082. Check for unnecessary newlines in config defaults#10840

HDDS-8082. Check for unnecessary newlines in config defaults#10840
cychiu8 wants to merge 2 commits into
apache:masterfrom
cychiu8:HDDS-8082

cychiu8 commented Jul 22, 2026

Uh oh!

cychiu8 commented Jul 22, 2026

Uh oh!

chihsuan left a comment

Uh oh!

chihsuan Jul 22, 2026

Uh oh!

cychiu8 Jul 23, 2026

Uh oh!

chihsuan Jul 22, 2026

Uh oh!

cychiu8 Jul 23, 2026

Uh oh!

chihsuan Jul 22, 2026

Uh oh!

cychiu8 Jul 23, 2026

Uh oh!

chihsuan left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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!

chihsuan Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

cychiu8 Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

chihsuan Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

cychiu8 Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

chihsuan Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

cychiu8 Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

chihsuan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants