[
https://issues.apache.org/jira/browse/YARN-6677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16493085#comment-16493085
]
Miklos Szegedi commented on YARN-6677:
--------------------------------------
Thank you, [~haibochen] for the patch.
{code:java}
// Reverse order by start time{code}
I think this should mention launch time
{code}
274 DefaultOOMHandler handler = new DefaultOOMHandler(context, false) {
275 @Override
276 protected CGroupsHandler getCGroupsHandler() {
277 return cGroupsHandler;
278 }
279 };
{code}
This could be solved with an overridden {{run()}} that calls the parent's run
in after overriding cgroups handler. The smaller amount of functions we have
the faster the code can be understood.
{code}OverAllocationOOMHandler.java{code}
I am not so convinced that it is a good idea to add another derived class here.
Could you just update DefaultOOMHandler with your code? Oversubscription and
opportunistic containers are first class citizens in Hadoop, we do not need to
add the logic as a plugin. If the logic works with all guaranteed containers, I
am fine updating {{DefaultOOMHandler.run()}}
{code}
9 * <p>
10 * http://www.apache.org/licenses/LICENSE-2.0
11 * <p>
{code}
I am not sure whether <p> is the standard.
{code}
40 * @param testVirtual Test virtual memory or physical
{code}
This was my mistake, but this time this parameter would deserve a more
meaningful name.
{code}
72 candidates.sort(CONTAINER_START_TIME_COMPARATOR);
{code}
I think we could write a very simple code that follows the logic, if we had a
two level comparator, that sorts by opportunistic flag first an then by launch
time descending.
{code}
93 Container c3 = createContainer(currentContainerId++,true, 2);
{code}
Missing space, also it might make sense to use launch times 1,2,3 instead of
1,2,2 keeping this 2 obviously.
{code}
309 @Test(expected = YarnRuntimeException.class)
310 public void testOOMUnresolvedAfterKillingAllContainers() throws
Exception {
{code}
This is probably my fault but this might need a good javadoc.
{code}
885 @Override
886 public long getContainerLaunchTime() {
887 return this.containerLaunchStartTime;
888 }
{code}
It would also be super useful to have a javadoc here explaining the difference
between start and launch times.
> Preempt opportunistic containers when root container cgroup goes over memory
> limit
> ----------------------------------------------------------------------------------
>
> Key: YARN-6677
> URL: https://issues.apache.org/jira/browse/YARN-6677
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager
> Affects Versions: 3.0.0-alpha3
> Reporter: Haibo Chen
> Assignee: Miklos Szegedi
> Priority: Major
> Attachments: YARN-6677.00.patch
>
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
