-
Notifications
You must be signed in to change notification settings - Fork 9.2k
YARN-10355. Refactor NM ContainerLaunch.java#orderEnvByDependencies #3220
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. |
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 for working on this issue @tomicooler. I think the general functionality is well done, the tests were already quite detailed regarding edge cases, so there should not be any surprise. Since this refactor is meant to maintain the code to be more readable, I advised some enhancement regarding this aspect.
...ava/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
Outdated
Show resolved
Hide resolved
...doop/yarn/server/nodemanager/containermanager/launcher/TestContainerLaunchParameterized.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
Outdated
Show resolved
Hide resolved
...doop/yarn/server/nodemanager/containermanager/launcher/TestContainerLaunchParameterized.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
brumi1024
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.
@tomicooler thanks for taking this on, good work on the refactor :) In general the change looks good to me, I had some minor comments.
...ava/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
Outdated
Show resolved
Hide resolved
...doop/yarn/server/nodemanager/containermanager/launcher/TestContainerLaunchParameterized.java
Outdated
Show resolved
Hide resolved
...doop/yarn/server/nodemanager/containermanager/launcher/TestContainerLaunchParameterized.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@tomicooler Thanks for the update! No additional comments on my side, +1 non-binding. |
|
Thanks for the updates, +1 from my side as well. |
|
🎊 +1 overall
This message was automatically generated. |
...ava/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
Outdated
Show resolved
Hide resolved
8251f55 to
f7a369f
Compare
|
Force pushed the branch.
NOTE: the new regex batch variable name extraction works for the %:% variable, it will extract the : as the name. The original code did not find the : character to be a variable name. (https://stackoverflow.com/questions/37973141/colon-in-batch-variable-name/37992843). Since the test refactor is moved to separate pull request, I will add an extra test case with %:% to this patch if the other pull request is merged before this change. |
|
💔 -1 overall
This message was automatically generated. |
|
@tomicooler checked the latest state, other than the two small checkstyle issues +1 from my side. |
Change-Id: If26d4aa33492903aaf24100fea4183a18e7d22ff
…ncies. Change-Id: If2ec5d61d64b311d9c7f0214e6aad59217095bfe
…ncies. Change-Id: If7e812975a108cc28c1fe6e1784a9840e60c09ce
f7a369f to
822f72c
Compare
|
💔 -1 overall
This message was automatically generated. |
Change-Id: I545c55bc1fa1eb2329f3d46590b793d5e237d587
|
Thanks @tomicooler for working on this. |
|
🎊 +1 overall
This message was automatically generated. |
|
@tomicooler thak you for the patch, LGTM+1. |
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