[jira] [Commented] (YARN-10674) fs2cs: should support auto created queue deletion.

[Peter Bacsko (Jira)]

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

Peter Bacsko commented on YARN-10674:
-------------------------------------

Thanks [~zhuqi] this is definitely looks better. We're close to the final 
version.

Some comments:
 1.
{noformat}
Disable the preemption with nopolicy or observeonly mode, " +
                    "default mode is nopolicy with no arg." +
                    "When use nopolicy arg, it means to remove " +
                    "ProportionalCapacityPreemptionPolicy for CS preemption, " +
                    "When use observeonly arg, " +
                    "it means to set " +
                    
"yarn.resourcemanager.monitor.capacity.preemption.observe_only " +
                    "to true"
{noformat}
I'd to slightly modify this text:
{noformat}
Disable the preemption with \"nopolicy\" or \"observeonly\" mode.
Default is \"nopolicy\".
\"nopolicy\" removes ProportionalCapacityPreemptionPolicy from
the list of monitor policies.
\"observeronly\" sets 
\"yarn.resourcemanager.monitor.capacity.preemption.observe_only\"
to true.
{noformat}
2. This definition:
 {{private String disablePreemptionMode;}}

This should be a simple enum like:
{noformat}
public enum DisablePreemptionMode {
  OBSERVE_ONLY {
    @Override
    String getCliOption() {
      return "observeonly";
    }
  },
  NO_POLICY {
    @Override
    String getCliOption() {
      return "nopolicy";
    }
  };

  abstract String getCliOption();
}
{noformat}
So you can also use them here:
{noformat}
 private static void checkDisablePreemption(CliOption cliOption,
              String disablePreemptionMode) {
            if (disablePreemptionMode == null ||
                disablePreemptionMode.trim().isEmpty()) {
              // The default mode is nopolicy.
              return;
            }
        
            try {
              DisablePreemptionMode.valueOf(disablePreemptionMode);
            } catch (IllegalArgumentException e) {
              throw new PreconditionException(
                  String.format("Specified disable-preemption option %s is 
illegal, " +
                      " use \"nopolicy\" or \"observeonly\""));
            }
{noformat}
"disablePreemptionMode" should be an enum everywhere.

3.
{noformat}
          public void convertSiteProperties(Configuration conf,
              Configuration yarnSiteConfig, boolean drfUsed, boolean 
enableAsyncScheduler) 
              boolean enableAsyncScheduler, boolean userPercentage,
              boolean disablePreemption, String disablePreemptionMode) {
{noformat}
Here "disablePreemptionMode" should be an enum also and make sure that it 
always has a value. If it always has a value, this part becomes much simpler:
{noformat}
              if (disablePreemption && 
                  disablePreemptionMode == DisablePreemptionMode.NO_POLICY) {
                
yarnSiteConfig.set(YarnConfiguration.RM_SCHEDULER_MONITOR_POLICIES, "");
              }
            }
{noformat}
4.
 {{AutoCreatedQueueDeletionPolicy.class.getCanonicalName())}}

This string is referenced very often in the tests. Instead, use a final String:
{noformat}
private static final String DELETION_POLICY_CLASS =
   AutoCreatedQueueDeletionPolicy.class.getCanonicalName();
{noformat}
So the readability becomes much better.

> fs2cs: should support auto created queue deletion.
> --------------------------------------------------
>
>                 Key: YARN-10674
>                 URL: https://issues.apache.org/jira/browse/YARN-10674
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Qi Zhu
>            Assignee: Qi Zhu
>            Priority: Major
>              Labels: fs2cs
>         Attachments: YARN-10674.001.patch, YARN-10674.002.patch, 
> YARN-10674.003.patch, YARN-10674.004.patch, YARN-10674.005.patch, 
> YARN-10674.006.patch, YARN-10674.007.patch, YARN-10674.008.patch, 
> YARN-10674.009.patch, YARN-10674.010.patch
>
>
> In FS the auto deletion check interval is 10s.
> {code:java}
> @Override
> public void onCheck() {
>   queueMgr.removeEmptyDynamicQueues();
>   queueMgr.removePendingIncompatibleQueues();
> }
> while (running) {
>   try {
>     synchronized (this) {
>       reloadListener.onCheck();
>     }
> ...
> Thread.sleep(reloadIntervalMs);
> }
> /** Time to wait between checks of the allocation file */
> public static final long ALLOC_RELOAD_INTERVAL_MS = 10 * 1000;{code}



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