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

[Andras Gyori (Jira)]

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

Andras Gyori commented on YARN-10674:
-------------------------------------

Overall logic seems good to me, this is the safest choice of all. I have some 
objections, however, sorry for coming up with these (none of them is a serious 
issue though):
 * definition of disablePreemptionMode is very hard to reason about, my 
suggestion is to change DisablePreemptionMode like (I think this is also a more 
idiomatic and flexible usage of Java enums)
{code:java}
public enum DisablePreemptionMode {
     NO_POLICY("nopolicy"),
     OBSERVE_ONLY("observeonly");

    private String cliOption;

    DisablePreemptionMode(String cliOption) {
      this.cliOption = cliOption;
    }

    public String getCliOption() {
      return cliOption;
    }
    
    public static DisablePreemptionMode fromString(String cliOption) {
      if (!StringUtils.isEmpty(cliOption) && 
cliOption.equals(DisablePreemptionMode.OBSERVE_ONLY.getCliOption())) {
        return DisablePreemptionMode.OBSERVE_ONLY;
      } else {
        return DisablePreemptionMode.NO_POLICY;
      }
    }
  }
{code}

 * This way disablePreemptionMode definition reduces to:
{code:java}
DisablePreemptionMode disablePreemptionMode =
        DisablePreemptionMode.fromString(disableModeString);
{code}

 * checkDisablePreemption has an unnecessary String.format call (I think a 
simple String is sufficient here)
 * Also since we have already implemented an enum, I was wondering whether it 
was worth merging the disablePreemption switch to the DisablePreemptionMode 
enum? They are always mutually exclusive, therefore we always check the 
DisablePreemptionMode along with the disablePreemption switch. This change is 
not necessary, but my suggestion would be to rename DisablePreemptionMode to 
PreemptionMode with the following entries:
{code:java}
public enum PreemptionMode {
     ENABLE("enable"),
     DISABLE_NO_POLICY("nopolicy"),
     DISABLE_OBSERVE_ONLY("observeonly");
{code}

 * import static org.junit.Assert.* in TestFSConfigConverter (wildcard import)
 * {color:#bf9100}testSiteDisabledPreemptionWithObserveOnlyConversion{color} 
assertEquals could be simplified to assertTrue

> 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, YARN-10674.011.patch, 
> YARN-10674.012.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