[jira] [Commented] (YARN-2962) ZKRMStateStore: Limit the number of znodes under a znode

["Daniel Templeton (JIRA)" <ji...@apache.org>]

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

Daniel Templeton commented on YARN-2962:
----------------------------------------

Thanks for updating the patch, [~varun_saxena].

The overall approach appears faithful to discussion above.  One optimization I might consider is to only add the splits that actually exist to the {{rmAppRootHierarchies}} since I would assume that the common case will be to not use splits.

Implementation comments:

Could you also explain in the parameter description why one would want to change it from the default of 0 and how to know what a good split value would be?

{code}
  static final int NO_APPID_NODE_SPLIT = 0;
{code}

I'm not sure this constant adds anything.  I found it made the code harder to read than just hard-coding in 0.  At a minimum, the name could be improved.  Took me a bit to realize you you weren't abbreviating "number" as "no".  At the absolute barest minimum, a comment to explain the constant would help.


{code}
  private final static class AppNodeSplitInfo {
    private final String path;
    private final int splitIndex;
    AppNodeSplitInfo(String path, int splitIndex) {
      this.path = path;
      this.splitIndex = splitIndex;
    }
    public String getPath() {
      return path;
    }
    public int getSplitIndex() {
      return splitIndex;
    }
  }
{code}

It may be just me, but for a private holding class like this, I don't think the accessors are needed.  Just access the member vars directly.


{code}
    if (appIdNodeSplitIndex < 1 || appIdNodeSplitIndex > 4) {
      appIdNodeSplitIndex = NO_APPID_NODE_SPLIT;
    }
{code}

This violates the Principle of Least Astonishment.  At least log a warning that you're not doing what the user said to.  I might even log it as an error.


{code}
    rmAppRootHierarchies = new HashMap<Integer, String>(5);
{code}

should be

{code}
    rmAppRootHierarchies = new HashMap<>(5);
{code}


{code}
      if (alternatePathInfo == null) {
        // Unexpected. Assume that app attempt has been deleted.
        return;
      }
      appIdRemovePath = alternatePathInfo.getPath();
{code}

I'm not a fan of the if-return style of coding.  I'd rather you did:

{code}
      // Assume that app attempt has been deleted if the path info is null
      if (alternatePathInfo != null) {
        appIdRemovePath = alternatePathInfo.getPath();
      }
{code}

and then wrap the tail of the method in {{if (appIdRemovePath != null) {}}.  Same in {{removeApplicationStateInternal()}} and {{removeApplication()}}.

Please include messages in your @throws comments.


{code}
  /**
   * Deletes the path. Assumes that path exists.
   * @param path Path to be deleted.
   * @throws Exception
   */
  private void safeDeleteIfExists(final String path) throws Exception {
    SafeTransaction transaction = new SafeTransaction();
    transaction.delete(path);
    transaction.commit();
  }

  /**
   * Deletes the path. Checks for existence of path as well.
   * @param path Path to be deleted.
   * @throws Exception
   */
   private void safeDelete(final String path) throws Exception {
     if (exists(path)) {
      safeDeleteIfExists(path);
     }
   }
{code}

What I see is that {{safeDelete()}} deletes the path if it exists, and {{safeDeleteIfExists()}} deletes the path blindly.  Might want to swap those method names.


{code}
  private AppNodeSplitInfo getAlternatePath(String appId) throws Exception {
    for (Map.Entry<Integer, String> entry : rmAppRootHierarchies.entrySet()) {
      // Look for other paths
      int splitIndex = entry.getKey();
      if (splitIndex != appIdNodeSplitIndex) {
        String alternatePath =
            getLeafAppIdNodePath(appId, entry.getValue(), splitIndex, false);
        if (exists(alternatePath)) {
          return new AppNodeSplitInfo(alternatePath, splitIndex);
        }
      }
    }
    return null;
  }
{code}

Näive question: what happens if an app happens to exist in more than one split?  I know that's not the expected case, but never underestimate the users...

I would also love to see some use of newlines to make the code a little more readable.

I would love to see javadoc comments on your test methods.

{code}
      HashMap<ApplicationAttemptId, RMAppAttempt> attempts =
          new HashMap<ApplicationAttemptId, RMAppAttempt>();
{code}

should be

{code}
      HashMap<ApplicationAttemptId, RMAppAttempt> attempts = new HashMap<>();
{code}


Your assert methods should have some message text to explain what went wrong.

It would be really swell if those two long test methods had some more explanatory comments so that they're easier to understand.

> ZKRMStateStore: Limit the number of znodes under a znode
> --------------------------------------------------------
>
>                 Key: YARN-2962
>                 URL: https://issues.apache.org/jira/browse/YARN-2962
>             Project: Hadoop YARN
>          Issue Type: Improvement
>          Components: resourcemanager
>    Affects Versions: 2.6.0
>            Reporter: Karthik Kambatla
>            Assignee: Varun Saxena
>            Priority: Critical
>         Attachments: YARN-2962.01.patch, YARN-2962.04.patch, YARN-2962.2.patch, YARN-2962.3.patch
>
>
> We ran into this issue where we were hitting the default ZK server message size configs, primarily because the message had too many znodes even though they individually they were all small.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)