-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-18748 Optimize Configuration.handleDeprecation #5685
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
b46d00c to
6db71fe
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
2a58c7f to
f8a8bba
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
you know, if we could speed up the basic .get() it can only be good. at the same time, this is such a broadly used piece of code, we are scared of doing anything risky to it. what can we do in terms of testing that all this is good? I'm thinking of concurrency as well as everything else? |
That's a good point. I have specifically avoided a larger rewrite to reduce the risks in breaking the implementation. Testing is quite wide since Can you advice on how to apply this change in other branches as well such that older versions can receive the benefits? |
f8a8bba to
4ebb9de
Compare
|
💔 -1 overall
This message was automatically generated. |
|
regarding older versions, once in trunk it can be cherry picked into branch-3.3 to ship in 3.3.9; you'd do that by submitting a new PR against the branch with the patch cherrypicked from trunk to it...that lets yetus do its reviewing again. We wouldn't expect to do any code review at this point, just validate the backport worked |
steveloughran
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.
I only have some minor changes here. but will try to seek broader review as the blast radius of a regression spans so many projects.
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java
Outdated
Show resolved
Hide resolved
4ebb9de to
601c265
Compare
|
💔 -1 overall
This message was automatically generated. |
|
Ah this broke after addressing the first round of comments and I accidentally introduced a bug. Fixed now.
Sure. I will merge instead if that's more convenient. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
ok, style failures the overlay one needs new variable name. the ConfigurationBenchmark one looks like one of the PRs added a file, which somehow is still around for the style checking. Probably the strategy there is actually do a squash commit and forced write, so we are down to a single patch. I know, it's not "elegant" but it ensures that there's no memory of a transient file |
593c3c9 to
c44a6a5
Compare
Fixed the style failures and rewrote the history to sidestep the accidental addition of the file. |
|
💔 -1 overall
This message was automatically generated. |
|
@steveloughran any more thoughts on this? |
|
We're closing this stale PR because it has been open for 100 days with no activity. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
Description of PR
Optimize
Configuration.handleDeprecationby adding a fastpath for when a property is not deprecated. While at it factor out property propagation so that it can be reused by both main and overlay properties.How was this patch tested?
Existing tests:
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?