[jira] [Comment Edited] (YARN-10043) FairOrderingPolicy Improvements
[
https://issues.apache.org/jira/browse/YARN-10043?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17035442#comment-17035442
]
Szilard Nemeth edited comment on YARN-10043 at 2/12/20 3:27 PM:
Hi [~maniraj...@gmail.com],
Some comments:
1. The javadoc of FairOrderingPolicy could be enhanched with specifying
"Compare using job submit time" a bit more: As per the implementation (compare
method), the app which is submitted before another app is preferred. This fact
should be included in the javadoc as well.
2. Still in the same javadoc: "3. Compare demands. SchedulableEntities without
resource demand get lower priority than ones who have demands." --> I would
change "who" to "which" or "that".
3. In TestFairOrderingPolicy: Multiple occurrences of statements like:
{code}
FairOrderingPolicy policy =
new FairOrderingPolicy();
{code}
You can remove the type argument to FairOrderingPolicy.
4. In TestFairOrderingPolicy: Multiple occurrences of statements like:
{code}
Assert.assertTrue(policy.getComparator().compare(r1, r2) == 0);
{code}
Should be changed to:
{code}
assertEquals(0, policy.getComparator().compare(r1, r2));
{code}
Please also add a message to all the assert statements.
5. In TestFairOrderingPolicy#testOrderingUsingAppDemand: You have multiple
occurrences of statements like:
{code}
r1.setUsed(Resources.createResource(0 * GB));
{code}
0*GB can be changed to 0.
6. In TestFairOrderingPolicy#testOrderingUsingAppDemand: I think you don't need
this statement:
{code}
// R1, R2 has been started at same time
assertEquals(r1.getStartTime(), r2.getStartTime());
{code}
as this assertion was already included in testOrderingUsingAppSubmitTime
was (Author: snemeth):
Hi [~maniraj...@gmail.com],
Some comments:
# The javadoc of FairOrderingPolicy could be enhanched with specifying
"Compare using job submit time" a bit more: As per the implementation (compare
method), the app which is submitted before another app is preferred. This fact
should be included in the javadoc as well.
# Still in the same javadoc: "3. Compare demands. SchedulableEntities without
resource demand get lower priority than ones who have demands." --> I would
change "who" to "which" or "that".
# In TestFairOrderingPolicy: Multiple occurrences of statements like:
{code}
FairOrderingPolicy policy =
new FairOrderingPolicy();
{code}
You can remove the type argument to FairOrderingPolicy.
# In TestFairOrderingPolicy: Multiple occurrences of statements like:
{code}
Assert.assertTrue(policy.getComparator().compare(r1, r2) == 0);
{code}
Should be changed to:
{code}
assertEquals(0, policy.getComparator().compare(r1, r2));
{code}
Please also add a message to all the assert statements.
# In TestFairOrderingPolicy#testOrderingUsingAppDemand: You have multiple
occurrences of statements like:
{code}
r1.setUsed(Resources.createResource(0 * GB));
{code}
0*GB can be changed to 0.
# In TestFairOrderingPolicy#testOrderingUsingAppDemand: I think you don't need
this statement:
{code}
// R1, R2 has been started at same time
assertEquals(r1.getStartTime(), r2.getStartTime());
{code}
as this assertion was already included in testOrderingUsingAppSubmitTime
> FairOrderingPolicy Improvements
> ---
>
> Key: YARN-10043
> URL: https://issues.apache.org/jira/browse/YARN-10043
> Project: Hadoop YARN
> Issue Type: Sub-task
>Reporter: Manikandan R
>Assignee: Manikandan R
>Priority: Major
> Attachments: YARN-10043.001.patch, YARN-10043.002.patch,
> YARN-10043.003.patch
>
>
> FairOrderingPolicy can be improved by using some of the approaches (only
> relevant) implemented in FairSharePolicy of FS. This improvement has
> significance in FS to CS migration context.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Comment Edited] (YARN-10043) FairOrderingPolicy Improvements
[
https://issues.apache.org/jira/browse/YARN-10043?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17028478#comment-17028478
]
Manikandan R edited comment on YARN-10043 at 2/2/20 4:47 PM:
-
[~pbacsko] Thanks for your reviews.
# Taken care.
# Compare demands ensures entity without resource demand get lower priority
than ones who have demands. When both entities has certain demands ( > 0), then
there is no actual comparison. Hence the changes are in that way. It has been
commented too at class level. Similar to {{FairSharePolicy}} implementation.
# For #3 and #4, yes there are multiple asserts intended to clearly show the
expected final ones are passing after asserting all earlier comparison (in the
precedence order) has been passed as well. However, I see your point and made
the changes by balancing both the views.
was (Author: maniraj...@gmail.com):
[~pbacsko] Thanks for your reviews.
# Taken care.
# Compare demands ensures entity without resource demand get lower priority
than ones who have demands. When both entities has certain demands ( > 0), then
there is no actual comparison. Hence the changes are in that way. It has been
commented too at class level. Similar to {{FairSharePolicy}} implementation.
# For #3 and #4, yes there are multiple asserts intended to clearly show the
expected final ones are passing after asserting all earlier comparison (in the
precedence order) has been passed as well. However, I see your point and made
the changes by balancing both the views.
> FairOrderingPolicy Improvements
> ---
>
> Key: YARN-10043
> URL: https://issues.apache.org/jira/browse/YARN-10043
> Project: Hadoop YARN
> Issue Type: Sub-task
>Reporter: Manikandan R
>Assignee: Manikandan R
>Priority: Major
> Attachments: YARN-10043.001.patch, YARN-10043.002.patch,
> YARN-10043.003.patch
>
>
> FairOrderingPolicy can be improved by using some of the approaches (only
> relevant) implemented in FairSharePolicy of FS. This improvement has
> significance in FS to CS migration context.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Comment Edited] (YARN-10043) FairOrderingPolicy Improvements
[
https://issues.apache.org/jira/browse/YARN-10043?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17025899#comment-17025899
]
Peter Bacsko edited comment on YARN-10043 at 1/29/20 2:06 PM:
--
Some comments from me:
1. Nit: Pay attention to missing white spaces, like {{if(res) == 0}} (after
"if")
2. {{compareDemand()}} can be simplified:
{noformat}
return (int) Math.signum(demand2 - demand1);
{noformat}
3. {{testOrderingUsingAppSubmitTime()}} has multiple asserts. I'd prefer having
separate test cases for better readability. Examples:
* testOrderingWithoutUsedAndPendingResources
* testOrderingWithUsedAndPendingResources
* testOrderingWithSubmissionTime
4. Same applies to {{testOrderingUsingAppDemand()}}. Could be split up like:
* testOrderingWithZeroDemand
* testOrderingWithSameStartTimeDifferentDemand
* Also, "//No changes, equal" part is the same as in
{{testOrderingUsingAppSubmitTime()}}
was (Author: pbacsko):
Some comments from me:
1. Nit: Pay attention to missing white spaces, like {{if(res) == 0}} (after
"if")
2. {{compareDemand()}} can be simplified:
{noformat}
return (int) Math.signum(demand2 - demand1);
{noformat}
3. {{testOrderingUsingAppSubmitTime()}} has multiple asserts. I'd prefer having
separate test cases for better readability. Examples:
* testOrderingWithoutUsedAndPendingResources
* testOrderingWithUsedAndPendingResource
* testOrderingWithSubmissionTime
4. Same applies to {{testOrderingUsingAppDemand()}}. Could be split up like:
* testOrderingWithZeroDemand
* testOrderingWithSameStartTimeDifferentDemand
* Also, "//No changes, equal" part is the same as in
{{testOrderingUsingAppSubmitTime()}}
> FairOrderingPolicy Improvements
> ---
>
> Key: YARN-10043
> URL: https://issues.apache.org/jira/browse/YARN-10043
> Project: Hadoop YARN
> Issue Type: Sub-task
>Reporter: Manikandan R
>Assignee: Manikandan R
>Priority: Major
> Attachments: YARN-10043.001.patch, YARN-10043.002.patch
>
>
> FairOrderingPolicy can be improved by using some of the approaches (only
> relevant) implemented in FairSharePolicy of FS. This improvement has
> significance in FS to CS migration context.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
