-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HADOOP-17358. Improve excessive reloading of Configurations #2436
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
|
💔 -1 overall
This message was automatically generated. |
|
those two Junit test are broken
|
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 would really appreciate if you rewrite this so it doesn't use -1 as startIdx. A year from now I won't remember why we want to load from position -1.
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.
Good point @jojochuang .I will think about something. or maybe addd a comment.
cd816f3 to
d6e5526
Compare
|
💔 -1 overall
This message was automatically generated. |
|
Thanks @jojochuang . I updated the PR. |
|
There is an open jira to fix |
| } | ||
| } | ||
| return properties; | ||
| return props; |
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 thought I posted a comment here, but somehow it got lost in github comments)
returning this object isn't necessary, and obviously the callers don't use the return value.
| * @param startIdx the index where the new resource has been added. | ||
| * @param fullReload flag whether we do complete reload of the conf instead | ||
| * of just loading the new resource. | ||
| * @return the properties loaded from the resource. |
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.
Could you also remove this @return? I am +1 once this is updated.
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.
Sorry . I overlooked that.
I added a new commit.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +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 merging it.
|
Merged into trunk. I'll cherry pick the commit into lower branches later. |
Co-authored-by: ahussein <[email protected]> (cherry picked from commit 71071e5) (cherry picked from commit 23fe3bd) (cherry picked from commit ac3e10a)
Co-authored-by: ahussein <[email protected]> (cherry picked from commit 71071e5) (cherry picked from commit 23fe3bd)
Co-authored-by: ahussein <[email protected]> (cherry picked from commit 71071e5) (cherry picked from commit 23fe3bd) (cherry picked from commit ac3e10a) (cherry picked from commit 487eaa6)
Co-authored-by: ahussein <[email protected]> (cherry picked from commit 71071e5)
Co-authored-by: ahussein <[email protected]> (cherry picked from commit 71071e5) (cherry picked from commit 23fe3bd)
) Co-authored-by: ahussein <[email protected]> (cherry picked from commit 71071e5) (cherry picked from commit 23fe3bd) (cherry picked from commit ac3e10a) (cherry picked from commit 487eaa6) Change-Id: Ia2c2f46f9437c70e1119ab8f0920aaeb5bb74a14 (cherry picked from commit 261437c7279b21e3190a2acbaba30fc0b21eb198)
NOTICE
Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HADOOP-XXXXX. Fix a typo in YYY.)
For more details, please see https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute