Skip to content

Conversation

@adoroszlai
Copy link
Contributor

What changes were proposed in this pull request?

Some config properties have embedded newlines, which breaks any output where they are included (e.g. #4271 (comment)).

This patch removes such newlines.

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

How was this patch tested?

Full CI:
https://github.com/adoroszlai/hadoop-ozone/actions/runs/4292591337

@adoroszlai adoroszlai self-assigned this Feb 28, 2023
Copy link
Member

@ayushtkn ayushtkn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, but how will you stop this in future? Can we add a test, like which loads the ozone-default, iterates over all the config values and checks for newlines?

@adoroszlai
Copy link
Contributor Author

Thanks @ayushtkn for the suggestion. I'm not sure we want to enforce such restriction. The whitespace being removed were present only for XML formatting for human readers, but there might be legitimate reasons for having them in other config values. The effect of newlines is cosmetic only, multiple log lines instead of one. #4271 will also handle the most common case (leading/trailing whitespace) by trimming before writing to log.

Copy link
Member

@ayushtkn ayushtkn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through. #4271, I thought your intention was to prevent developers from doing this even for cosmetic reasons or to be precise readability reasons and not push things to be sorted like done as part of #4721.

So, 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.

But I am ok, with whatever way you feel good. So, do you want to proceed with this change itself or let the other PR handle this case as well?

The change is safe and harmless so LGTM, commit at your discretion.

@adoroszlai adoroszlai merged commit 1af6e91 into apache:master Mar 5, 2023
@adoroszlai adoroszlai deleted the HDDS-8046 branch March 5, 2023 08:30
@adoroszlai
Copy link
Contributor Author

Thanks @ayushtkn, created HDDS-8082 for the improvement you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants