-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HADOOP-18021. Provide a public wrapper of Configuration#substituteVars #3710
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
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.
oops. At least its surfaced before things shipped -right?
made a hadoop- JIRA as its only touching hadoop-common; the final commit message should refer to the yarn issues which caused it, so that git log --grep for those issues find this too.
You will need a test! just to make sure the method continues to work in future.
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
Show resolved
Hide resolved
Thanks for the suggestions @steveloughran. I am not sure about the tests however, what do you have in mind? Variable substitution is already tested in TestConfiguration#testVariableSubstitution. |
just make sure the new public method does some substitution -simply to make sure the wire up never breaks |
🎊 +1 overall
This message was automatically generated. |
@steveloughran I have added a new test case to provide a safety net. |
apache#3710) Contributed by Andras Gyori
…ubstituteVars (apache#3710) Contributed by Andras Gyori (cherry picked from commit 47ea0d7) Change-Id: I444ddba12442c039c8098b9ca0c312b86f1f0e37 (cherry picked from commit fd36e50)
Description of PR
How was this patch tested?
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?