[ https://issues.apache.org/jira/browse/YARN-6307?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16099134#comment-16099134 ]
Daniel Templeton commented on YARN-6307: ---------------------------------------- Nice patch, [~yufeigu]. Here are my comments: # Since we're unnesting all the _if_ blocks, let's take do that here, too: {code} if (res == 0) { // Apps are tied in fairness ratio. Break the tie by submit time and job // name to get a deterministic ordering, which is useful for unit tests. res = (int) Math.signum(s1.getStartTime() - s2.getStartTime()); if (res == 0) { res = s1.getName().compareTo(s2.getName()); } }{code} # Let's not stack the declarations: {code} double useToWeightRatio1, useToWeightRatio2;{code} Otherwise, looks good. > Refactor FairShareComparator#compare > ------------------------------------ > > Key: YARN-6307 > URL: https://issues.apache.org/jira/browse/YARN-6307 > Project: Hadoop YARN > Issue Type: Bug > Components: fairscheduler > Reporter: Yufei Gu > Assignee: Yufei Gu > Attachments: YARN-6307.001.patch, YARN-6307.002.patch > > > The method does three things: compare the min share usage, compare fair share > usage by checking weight ratio, break tied by submit time and name. They are > mixed with each other which is not easy to read and maintenance, poor style. > Additionally, there are potential performance issues, like no need to check > weight ratio if minShare usage comparison already indicate the order. It is > worth to improve considering huge amount invokings in scheduler. -- This message was sent by Atlassian JIRA (v6.4.14#64029) --------------------------------------------------------------------- To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org