[jira] [Comment Edited] (YARN-999) In case of long running tasks, reduce node resource should balloon out resource quickly by calling preemption API and suspending running task.

["Giovanni Matteo Fumarola (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/YARN-999?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16811171#comment-16811171 ] 

Giovanni Matteo Fumarola edited comment on YARN-999 at 4/5/19 7:06 PM:
-----------------------------------------------------------------------

Thanks [~elgoiri] for the hard work to add this important feature.
The patch is fully tested with good unit tests.`

Few comments on the last patch:
 * Few times you used containers launched. It should be launched containers.
 * Line 253 LOG.debug("{} is overcommitted ({}), preempt/kill containers", should be LOG.info. I would like to search in the code what the situation.
 * You can use the word Overcommitted over "over committed".
 * In testGetRunningContainersToKill some comments have the numeration missing. e.g AM instead of AM0.
 * Line 1069         false, ExecutionType.GUARANTEED, "GUARANTEED0"); it should be GUARANTEED1
 * I am not a fan of Static imports. when should you use static import? Very sparingly! Only use it when you'd otherwise be tempted to declare local copies of constants, or to abuse inheritance (the Constant Interface Antipattern). ... If you overuse the static import feature, it can make your program unreadable and unmaintainable, polluting its namespace with all the static members you import. Readers of your code (including you, a few months after you wrote it) will not know which class a static member comes from. Importing all of the static members from a class can be particularly harmful to readability; if you need only one or two members, import them individually.

[Static import documentation|https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html)]. I would remove those from the new tests classes.


was (Author: giovanni.fumarola):
Thanks [~elgoiri] for the hard work to add this important feature.
The patch is fully tested with good unit tests.`

Few comments on the last patch:
 * Few times you used containers launched. It should be launched containers.
 * Line 253 LOG.debug("{} is overcommitted ({}), preempt/kill containers", should be LOG.info. I would like to search in the code what the situation.
 * You can use the word Overcommitted over "over committed".
 * In testGetRunningContainersToKill some comments have the numeration missing. e.g AM instead of AM0.
 * Line 1069         false, ExecutionType.GUARANTEED, "GUARANTEED0"); it should be GUARANTEED1
 * I am not a fan of Static imports. when should you use static import? Very sparingly! Only use it when you'd otherwise be tempted to declare local copies of constants, or to abuse inheritance (the Constant Interface Antipattern). ... If you overuse the static import feature, it can make your program unreadable and unmaintainable, polluting its namespace with all the static members you import. Readers of your code (including you, a few months after you wrote it) will not know which class a static member comes from. Importing all of the static members from a class can be particularly harmful to readability; if you need only one or two members, import them individually.

([https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html)
]I would remove those from the new tests classes.

> In case of long running tasks, reduce node resource should balloon out resource quickly by calling preemption API and suspending running task. 
> -----------------------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: YARN-999
>                 URL: https://issues.apache.org/jira/browse/YARN-999
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: graceful, nodemanager, scheduler
>            Reporter: Junping Du
>            Assignee: Íñigo Goiri
>            Priority: Major
>         Attachments: YARN-291.000.patch, YARN-999.001.patch, YARN-999.002.patch, YARN-999.003.patch, YARN-999.004.patch, YARN-999.005.patch, YARN-999.006.patch, YARN-999.007.patch, YARN-999.008.patch, YARN-999.009.patch
>
>
> In current design and implementation, when we decrease resource on node to less than resource consumption of current running tasks, tasks can still be running until the end. But just no new task get assigned on this node (because AvailableResource < 0) until some tasks are finished and AvailableResource > 0 again. This is good for most cases but in case of long running task, it could be too slow for resource setting to actually work so preemption could be used here.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org