-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
HADOOP-18371. s3a FS init logs at warn if fs.s3a.create.storage.class is unset #4730
Conversation
Tested against
|
💔 -1 overall
This message was automatically generated. |
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.
+1 LGTM
Thanks @monthonk. |
LOG.warn("Unknown storage class property {}: {}; falling back to default storage class", | ||
LOG.info("Unknown storage class property {}: {}; falling back to default storage class", |
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.
If you don't configure anything and then if it is falling back to default, it should be info in that case, but if someone configured it and configured it wrong it should be warn only right? it isn't the normal flow of execution
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.
Ah, what a miss :(
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 @ayushtkn !
Testing in progress |
@@ -1001,8 +1001,13 @@ protected RequestFactory createRequestFactory() { | |||
try { | |||
storageClass = StorageClass.fromValue(storageClassConf); | |||
} catch (IllegalArgumentException e) { | |||
LOG.warn("Unknown storage class property {}: {}; falling back to default storage class", | |||
STORAGE_CLASS, storageClassConf); | |||
if (storageClassConf.length() == 0) { |
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.
Can do storageClassConf.isEmpty()
rather than calculating the length.
BTW. If it is empty you need not to attempt storageClass = StorageClass.fromValue(storageClassConf);
and then land up with this Exception handling
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.
Sounds good, much cleaner approach
Will share the test results in some time. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Endpoint:
When run individually, |
STORAGE_CLASS, storageClassConf); | ||
} | ||
} else { | ||
LOG.info("Empty storage class property {}; falling back to default storage class", |
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.
We print configuration related logs in debug only unless they are supposed to cause error. So I think this should be debug. right @steveloughran
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.
Also shouldn't the log message say " Unset" rather than empty?
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.
yes, debug only, apart from some special cases where there's a risk of incompatibility across versions (dir marker policy). even there i think it is time to retire it.
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.
looks good, just need that L1009 message at debug.
💔 -1 overall
This message was automatically generated. |
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.
LGTM +1,
Ignoring the yetus failure because of no new tests,
…class is unset (#4730) Contributed By: Viraj Jasani
pushed to branch-3.3 as well. |
…class is unset (apache#4730) Contributed By: Viraj Jasani
No description provided.