hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel Templeton (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-6885) AllocationFileLoaderService.loadQueue() should use a switch statement in the main tag parsing loop instead of the if/else-if/...
Date Thu, 10 Aug 2017 17:26:06 GMT

    [ 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-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message