YARN-11196. NUMA support in DCE #4742
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Samrat002
changed the title
|
💔 -1 overall
This message was automatically generated. |
| @@ -98,6 +104,13 @@ public DefaultContainerExecutor() { | |||
| } | |||
| } | |||
|
|
|||
| @VisibleForTesting | |||
There was a problem hiding this comment.
Looks this constructor is only for Testing purpose. Shall we use setMethod instead which is followed in other classes.
There was a problem hiding this comment.
removed the constructor and added setter which is set with annotation VisibleForTesting
| String[] command = getRunCommand(wrapperScriptPath, | ||
| containerIdStr, user, pidFile, this.getConf(), resource); | ||
|
|
||
| if(numaAwarenessEnabled(this.getConf())) { | ||
| try { | ||
| numaResourceAllocator.init(this.getConf()); |
There was a problem hiding this comment.
The numaResourceAllocator.init needs to be moved into init() method
| 0L, | ||
| false); | ||
| } | ||
|
|
||
| String[] addNumaCommands(String[] commands, Container container){ | ||
| try { | ||
| NumaResourceAllocation numaAllocation = numaResourceAllocator.allocateNumaNodes(container); |
There was a problem hiding this comment.
The allocateNumaNodes call look better if called from launchContainer instead of buildCommandExecutor.
|
🎊 +1 overall
This message was automatically generated. |
| 0L, | ||
| false); | ||
| } | ||
|
|
||
| String[] addNumaCommands(String[] commands, Container container){ | ||
| try { | ||
| NumaResourceAllocation numaAllocation = numaResourceAllocator.allocateNumaNodes(container); |
There was a problem hiding this comment.
Launch Container has to release the NUMA Resources at finally.
| 0L, | ||
| false); | ||
| } | ||
|
|
||
| String[] addNumaCommands(String[] commands, Container container){ | ||
| try { | ||
| NumaResourceAllocation numaAllocation = numaResourceAllocator.allocateNumaNodes(container); |
There was a problem hiding this comment.
Reacquire Container has to recover the NUMA Resources.
|
🎊 +1 overall
This message was automatically generated. |
This reverts commit 571a0657ccfbae3ef9dc0bd1dddc032008b45f91.
Samrat002
force-pushed
the
numa-for-dce
branch
from
0b874ad
to
360a7b0
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Samrat002
force-pushed
the
numa-for-dce
branch
from
088e31b
to
5e81142
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@PrabhuJoseph Thank you for reviewing the pr and providing with valuable comments. |
|
check style is failing due to accesive params passed in the func args ! |
| @@ -350,6 +384,14 @@ public int launchContainer(ContainerStartContext ctx) | |||
| return exitCode; | |||
| } finally { | |||
| if (shExec != null) shExec.close(); | |||
| try { | |||
There was a problem hiding this comment.
Shall we use the same postComplete method here as well.
Samrat002
force-pushed
the
numa-for-dce
branch
from
c5c9a8d
to
a16de69
Compare
|
💔 -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. |
Fix checkstyle error Remove container reference Adding test for the numa changes for DCE Remove unused variable creation releasing numa resources when container closes and reassign during resassign time Addressing to comments Fixing failed test case Fixing failed test case Fix extra spaces Add test for reaquiredContainer Fix test error correcting the test cases properly correcting the test cases properly correcting the test cases properly correcting the test cases properly correcting the test cases properly Putting extra logs
Samrat002
force-pushed
the
numa-for-dce
branch
from
1af441e
to
6172d5f
Compare
|
🎊 +1 overall
This message was automatically generated. |
| */ | ||
| String[] getNumaCommands(NumaResourceAllocation resourceAllocation) { | ||
| String[] numaCommand = new String[3]; | ||
| numaCommand[0] = this.getConf().get(YarnConfiguration.NM_NUMA_AWARENESS_NUMACTL_CMD, |
There was a problem hiding this comment.
Better to read this config and initialize in the init.
| when(mockContainer.getContainerId()).thenReturn( | ||
| ContainerId.fromString("container_1481156246874_0001_01_000004")); | ||
| when(mockContainer.getResource()) | ||
| .thenReturn(Resource.newInstance(156250, 2)); |
There was a problem hiding this comment.
Better to check for a lesser resource size (1024) instead of 156250.
| String[] command = getRunCommand(wrapperScriptPath, | ||
| containerIdStr, user, pidFile, this.getConf(), resource); | ||
|
|
||
| command = concatStringCommands(command, numaCommands); |
There was a problem hiding this comment.
Shall we skip calling this when numaCommands is not passed.
| * @return combined array of string where first elements are from firstStringArray | ||
| * and later are the elements from secondStringArray | ||
| */ | ||
| String[] concatStringCommands(String[] firstStringArray, String[] secondStringArray) { |
There was a problem hiding this comment.
The Numa Commands has to be in prefix.
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks @Samrat002 for the patch. The latest patch looks good to me, +1. Will commit it tomorrow if no other comments. |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
YARN-11196
How was this patch tested?
This patch is tested in emr cluster with 5 core nodes of m5.25xlarge machine and 1 master node of m5.12xlarge machine
Attaching portion of nodemanager logs
submitted example job
Logs for assigning numa resources
Logs for releasing numa resources
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?