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

Daniel Templeton commented on YARN-6885:
----------------------------------------

Thanks for the patch, [~Yu-Tang Lin].  Here are my comments:

# I don't see the value of declaring {{text}} and {{val}} outside the _switch_. 
 If saves some characters, but it has no runtime impact, and I think it makes 
the code a less clear, especially since you're having to cast {{val}} when 
using it.
# In {code}312          case "queueMaxResourcesDefault":
313               text = ((Text)element.getFirstChild()).getData().trim();
314               val =
315                     
FairSchedulerConfiguration.parseResourceConfigValue(text);
316               queueMaxResourcesDefault = (Resource)val;
317               break;{code} lines 314 and 315 can be combined.
# In {code}362            if (text.equalsIgnoreCase(FifoPolicy.NAME)) {
363                 throw new AllocationConfigurationException("Bad fair 
scheduler "
364                 + "config file: defaultQueueSchedulingPolicy or "
365                 + "defaultQueueSchedulingMode can't be FIFO.");
366                }
367                defaultSchedPolicy = SchedulingPolicy.parse(text);
368                break;{code} lines 364 and 365 should be indented 4 more 
spaces.
# In {code}545        case "weight":
546             {
547               text = ((Text)field.getFirstChild()).getData().trim();
548               double doubleVal = Double.parseDouble(text);
549               queueWeights.put(queueName, new 
ResourceWeights((float)doubleVal));
550             }
551             break;{code} Why the braces?  I don't think they're needed.
# Same here: {code}562        case "fairSharePreemptionThreshold":
563             {
564               text = ((Text)field.getFirstChild()).getData().trim();
565               float floatVal = Float.parseFloat(text);
566               floatVal = Math.max(Math.min(floatVal, 1.0f), 0.0f);
567               fairSharePreemptionThresholds.put(queueName, floatVal);
568             }
569             break;{code}
# In {{AllocationFileLoaderService}}, the last two _case_ statements don't 
work, because the _if_ statements they're replacing weren't testing equality.  
You'll have to keep the _if_ statements and put them in the _default_ case.

> AllocationFileLoaderService.loadQueue() should use a switch statement in the 
> main tag parsing loop instead of the if/else-if/...
> --------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: YARN-6885
>                 URL: https://issues.apache.org/jira/browse/YARN-6885
>             Project: Hadoop YARN
>          Issue Type: Improvement
>          Components: fairscheduler
>    Affects Versions: 3.0.0-alpha4
>            Reporter: Daniel Templeton
>            Assignee: Yu-Tang Lin
>            Priority: Minor
>              Labels: newbie
>             Fix For: 3.0.0-alpha4
>
>         Attachments: YARN-6885.005.patch
>
>
> {code}      if ("minResources".equals(field.getTagName())) {
>         String text = ((Text)field.getFirstChild()).getData().trim();
>         Resource val =
>             FairSchedulerConfiguration.parseResourceConfigValue(text);
>         minQueueResources.put(queueName, val);
>       } else if ("maxResources".equals(field.getTagName())) {
>       ...{code}



--
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

Reply via email to