-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-19763: Remove deprecated properties from core-default.xml #8148
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
|
@steveloughran , in HADOOP-19597, you expressed concerns about |
|
💔 -1 overall
This message was automatically generated. |
zabetak
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.
Would it make sense to add a unit test (maybe in org.apache.hadoop.conf.TestConfiguration or org.apache.hadoop.conf.TestConfigurationDeprecation) that loads core-default.xml and validates that it does not contain deprecated properties? Apart from verifying that the current PR is complete it could prevent someone from adding deprecated props in this file in the future.
Somewhat off-topic but ideally all xxx-default.xml files should be auto-generated via code/build scripts and not be manually curated. If that was the case we could avoid having this kind of changes now and in the future.
Closes apache#8148 Signed-off-by: Shilun Fan <[email protected]> Reviewed-by: Stamatis Zampetakis <[email protected]>
484305d to
ca78f32
Compare
|
@zabetak , great idea! I pushed up a new test. I'll do the same for YARN in #8149 . I don't have plans right now for adding test coverage of others (hdfs-default.xml, mapred-default.xml, distcp-default.xml, etc.). The same technique could apply there though if anyone wants to add those in the future. |
Closes apache#8148 Signed-off-by: Shilun Fan <[email protected]> Reviewed-by: Stamatis Zampetakis <[email protected]>
ca78f32 to
50a681e
Compare
|
💔 -1 overall
This message was automatically generated. |
|
The build failure is an unrelated pre-existing problem on trunk that I'm trying to fix in #8150 . |
|
💔 -1 overall
This message was automatically generated. |
| @Test | ||
| public void testNoDeprecationsByDefault() throws Exception { | ||
| // Force initialization to make sure deprecations are recorded for later calls to isDeprecated. | ||
| new Configuration(); |
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.
The test looks good although it can't find all deprecated properties (e.g., io.bytes.per.checksum) cause the respective config classes are not loaded. I logged HADOOP-19769 which may be a more complete solution to avoid deprecated props in config files. Obviously, this PR is already good enough and we don't need to strictly wait for HADOOP-19769. If there is interest about HADOOP-19769 let me know and I will try to finalize the POC.
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, HADOOP-19769 looks like a good idea to me!
cnauroth
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.
@slfan1989 , are you still +1 for this? I added a new unit test after your last reviewed.
aajisaka
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.
Thank you @cnauroth
|
I committed this to trunk. @slfan1989 , @zabetak and @aajisaka , thank you for the reviews. |
Description of PR
Recent trunk changes now produce more assertive warnings about use of deprecated configuration properties. With some of these deprecated properties included in core-default.xml, that means every process launched logs the warnings. Remove them from core-default.xml to avoid these warnings. (Note that this does not take the step of actually removing the code that uses the deprecated properties. They'll continue to work. We just don't want warnings logged by default if the user didn't specifically use these properties.)
How was this patch tested?
Build the distro:
Verify the command above no longer produces deprecation warnings.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?