-
Notifications
You must be signed in to change notification settings - Fork 594
HDDS-7869. Log configuration on component startup. #4271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
cc @sumitagrawl @dombizita @duongkame can you'll take a look as well? |
|
thanks @SaketaChalamchala - Looks to be some CI failures in the integration tests. |
sumitagrawl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SaketaChalamchala Thanks for working on this, one query is there related to completeness and correctness of configs getting logged.
...rver-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManagerStarter.java
Outdated
Show resolved
Hide resolved
|
|
||
| public static void startupShutdownMessage(VersionInfo versionInfo, | ||
| Class<?> clazz, String[] args, Logger log) { | ||
| Class<?> clazz, String[] args, Logger log, String conf) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to pass a Map here instead of a fully formatted string. This would allow filtering some sensitive properties if needed, as well as sorting the config properties alphabetically. (Currently order is random, since Properties is just a Hashtable.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing the configuration object as it is and I added a processForLogging utility function to redact sensitive properties and sort them before logging it.
|
I don't think we should filter by tags or prefixes when printing the config. I think we should just print the contents of whatever config object the component has.
Also +1 to Attila's suggestion of sorting the configs alphabetically. |
…d sort conf object for logging.
|
Thanks @errose28 @adoroszlai and @sumitagrawl for the review.
I'm printing all the properties in the config object used by each service which includes core-site, ozone-site and all the defaults. Below are examples. It seems like a lot. Let me know if it's readable. |
|
Similar list from other services |
|
Updated the patch to include Ozone configs only using regex. |
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java
Outdated
Show resolved
Hide resolved
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @SaketaChalamchala for updating the patch.
|
One minor question:
These newlines are the result of the whitespace embedded in some config properties' default values, e.g.: ozone/hadoop-hdds/common/src/main/resources/ozone-default.xml Lines 842 to 845 in e90e2dd
It would be nice to ensure |
It might be better to do trim the strings in |
errose28
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @SaketaChalamchala LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this ozoneregex not contains "dfs.datanode.use.datanode.hostname",so "dfs.datanode.use.datanode.hostname" init only once and not modifiy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These dfs configs are supposed to be deprecated, but it looks like Ozone is still using some of them instead of migrating them to its own config keys. We migrated most of them, but missed the ones in DFSConfigKeysLegacy. I filed HDDS-12026 to fix this. Would you like to work on it?
What changes were proposed in this pull request?
Added a
conffield inSTARTUP_MSGfor all Ozone roles with configs relevant to the role.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7869
How was this patch tested?
Manual test