Hi Karthik Kambatla, I'm interested to know more about this. Could you tell me any exception messages you are getting. From the description I'm thinking you are talking about the "jute.maxbuffer" java system property of ZooKeeper(defaulting to 1MB data size). As I know this property is used in the following cases:

1. znode data size - user added large data on a znode and submitting request to the ZooKeeper server.
2. say if there are n children under one znode(say /myapp). Now when user call zkc#getChildren(/myapp), server will get all the children and put into a list. This list will be added to the response object. If the list exceeds 1MB size (characters of all the znode names) will validate against the size and throws exception.

I hope you are talking about 2nd case?. Thanks!

Rakesh R - you are right, we saw the second case.

OK. Recently I had given a talk about ZooKeeper. Please refer ZooKeeper In The Wild presentation slides. In that I had mentioned similar case. Probably you guys can have a look at the slides and page no. 30. Here the idea is, instead of flat structure use hierarchical structure. For this user need to split the single znode name to form a hierarchy. With this approach user can store many znodes. AFAIK this is proven method in Apache BookKeeper project.

For example, application_1418470446447_0049 can split to form a hierarchy like, (app_root)\application_\141\84704\46447_\0049. If there is a data for this application, user can store it to the leaf znode. Since am not very good in yarn, you guys can find a better way to split the znode name for holding n number of znodes. Provide a parser to read it back by iterating the child znodes and form application_1418470446447_0049. Since ZooKeeper read operation is low latency one, it wont hit performance I guess. Probably we can do a test and see the performance graph.

-Rakesh

Rakesh R, thanks for your input. ApplicationID in YARN is of the format

application_[cluster timestamp]_[sequence number]

Here sequence number has 4 digits and is in the range 0000-9999.
Going along the lines of what you are saying, I think we can break the sequence number part of ApplicationID as cluster timestamp will probably be same for most of the application IDs'. My suggestion is to have it as

(app_root)\application_[cluster timestamp]_\[first 2 digits of sequence number]\[last 2 digits]

We can view it as under :

   * |--- RM_APP_ROOT
   * |     |----- (application_{cluster timestamp}_)
   * |     |        |----- (00 to 99)
   * |     |        |        |------ (00 to 99)
   * |     |        |        |         |----- (#ApplicationAttemptIds)

Rakesh R and Karthik Kambatla, kindly comment on the approach. One constraint is that this would entail a larger number of contacts to ZK when RM is recovering.
I am not sure how many znodes can lead to reaching limit of 1 MB. We can break sequence number as 1 digit and last 3 digit as well.

Moreover, I dont see much of an issue with application attempt znodes as max-attempts by default are limited to 2.

Karthik Kambatla, your views on this

The proposed approach sounds reasonable. How about adding a config that controls the number of digits (decimal places) to get at 2:2 or 1:3 split.

Yup, that sounds good.

Karthik Kambatla / Karthik Kambatla, for this I WILL assume that state store will be formatted before making the config change ?
Backward compatibility for running apps after config change (on RM restart) will be difficult. As we may have to try all the possible appid formats.

Varun Saxena, Yes, RMStateStore#checkVersion validates the version. I think this change can be a major upgrade of the version.

Varun Saxena - it will indeed be backwards incompatible. We should get this into trunk. To include this in branch-2, we should probably guard this with a config, and users will have to format the store to start using this. Otherwise, an RM restart will fail.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12701955/YARN-2962.01.patch
against trunk revision ca1c00b.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

-1 findbugs. The patch appears to introduce 5 new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager.

Test results: https://builds.apache.org/job/PreCommit-YARN-Build/6804//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/6804//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/6804//console

This message is automatically generated.

Findbugs to be addressed by YARN-3204

Karthik Kambatla / Jian He / Tsuyoshi Ozawa, kindly review

Suggest a better config name ?
Also currently when AppID format is not as per config, an exception is thrown which would lead to RM retrying to transition to active. Should we explicitly exit RM ? Thoughts ?

Varun Saxena, Thanks for the patch. It looks pretty good.. also like the extensive testing.

Couple of minor comments:

1. The getLeafAppIdNodePath(Sting appId) and getLeafAppIdNodePath(Sting appId, boolean createIfExists) looks pretty similar. They should probably be refactored so that the former calls the later with createIfExists = false ?
2. minor indentation errors (think you are using tabs) in loadRMAppState() and removeApplicationStateInternal()
3. in removeApplicationStateInternal(), cant you call getLeafAppIdNodePath instead ?

Also was wondering, should we hard code the NO_INDEX_SPLITTING logic to 4 ? Essentially, is it always guaranteed that sequence number will always be exactly 4 digits ? I was thinking we just allow users to specify a split index. If split index == size of seqnum, then don't split.. etc. Either way, I am ok with the current implementation, just wondering if it was though of (And I guess it might reduce some if then checks)

Thanks Arun Suresh for the review.

minor indentation errors (think you are using tabs) in loadRMAppState() and removeApplicationStateInternal()

Oh missed that one. Sorry for that. Will fix.

About refactoring part, I will look into it as to how I can refactor it.

Also was wondering, should we hard code the NO_INDEX_SPLITTING logic to 4 ? Essentially, is it always guaranteed that sequence number will always be exactly 4 digits ?

Yes it is not always guaranteed to be 4. It is minimum 4 digits and can go upto limit of integer which would be 10 digits.
But we have another configuration about maximum number of apps in state store which is by default 10000. So, effectively there wont be any more than this number of apps in state store. That is why I considered split index to be maximum 4. Also it is the simplest way of configuration. Considering upto 10 digits and having split index as > 4 would cause issues. And I could not think of a simpler config.

But thanks for pointing this out. Actually brought my attention towards an important issue. I think split index should carry out the split from the end of the sequence number.
Let us say, we have apps upto application_{cluster_timestamp}_9999. Next app would be application_{cluster_timestamp}_10000. As max number of apps in state store is 10000, application_{cluster_timestamp}_0000 would be deleted from state store.
Consider if split index is 2. If I count split index from beginning, applications from application_{cluster_timestamp}_10000 to application_{cluster_timestamp}_10999 would go under znode application_{cluster_timestamp}_10 where apps from application_{cluster_timestamp}_1000 to application_{cluster_timestamp}_1099 already exist. This can make the original problem(of exceeding jute maxbuffer size) to recur. Please note that apps application_{cluster_timestamp}_1000 to application_{cluster_timestamp}_1099 wont be deleted anytime soon and this would make this znode having a lot of children.

If we split it from the end though, application_{cluster_timestamp}_10000 to application_{cluster_timestamp}_10099 would go under znode application_{cluster_timestamp}_100 instead. I think this approach is correct instead of currently implemented one.

Yup.. agreed, star index from the end is a better

.. alternative to starting the index from the front.

Varun Saxena, wondering if you need any help with this. Would like to get this in soon.

A little busy nowadays. The change I suggested though wont take much time. Will try to complete over the weekend and update a patch.

Updating Patch:

• Rebased with trunk
• As discussed in the comments, the split index is now evaluated from the end of the appId.

-1 overall

This message was automatically generated.

Updating with a few minor fixes.

-1 overall

This message was automatically generated.

it will indeed be backwards incompatible. We should get this into trunk. To include this in branch-2, we should probably guard this with a config, and users will have to format the store to start using this. Otherwise, an RM restart will fail.

Apologies for coming in late. I don't think we should give up on compatibility that easily. You can easily provide compat by doing one of the following

• Forcing the new structure into its own parent directly. For e.g. instead of directly under RM_APP_ROOT, put it under something like RM_APP_ROOT/hierarchies, that way RM can read the old and new styles. /cc Jian He.
• If not that, we can simplify it without special hierarchies by having RM recursively read nodes all the time. As we already specifically look for "application_" prefix to read app-date, having application_1234_0456 right next to a 00/ directory is simply going to work without much complexity.

That way, we should also have a more reasonable default instead of the current no-split default config value.

we can simplify it without special hierarchies by having RM recursively read nodes all the time. As we already specifically look for "application_" prefix to read app-date, having application_1234_0456 right next to a 00/ directory is simply going to work without much complexity

Hmmm.. Currently, while reading, the code expects only leaf nodes to have data. We could modify it to continue to child nodes while loading RMState. But updates to an app state would require some thought. Consider updating state of app Id _1000000. The update code would have to first check both the /.._1000000 and /.._10000/00 znodes. Also, retrieving state during load_all and update_single might be hairy.. there can be ambiguous paths since a node path might not be unique across the 2 schemes. For eg. /.._10000 will exist both in the new and old scheme. In the old scheme it can contain data, but in the new scheme it shouldnt (it is an intermediate node for /.._10000/[00-99])..

Although option 2 can be done, I'd prefer the your first suggestion (storing under RM_APP_ROOT/hierarchies). We can have the RM read the old style but new apps and updates to old apps will go under the new root. We can even delete the old scheme root if no children exist.

Thinking more, the new RM moving to a hierarchy is also a problem with (rolling) downgrade. We may have to write a tool for admins to run when downgrading from this new scheme to the old scheme.

Canceling the patch for continuing the discussion..

Vinod Kumar Vavilapalli / Arun Suresh, supporting 2 separate hierarchies will increase complexity. Let us consider option 1 i.e. having RM_APP_ROOT/hierarchies. Here, we also need to consider the case where split index can be changed from say 2 to 3. To handle this case we can have multiple folders under hierarchies to indicate split index. But, this would mean that for an app we may have to look under upto 5 locations till we succeed.
Option 2 can also be done. Here we can check whether data exists under a znode or not to determine whether we found the app or not. Here also we may have to look up multiple times before finding an app though.

We can also do as under :
1. As Vinod suggested, write a tool or utility like "yarn resourcemanager -format-state-store" to migrate apps from the current scheme to the newly configured scheme. Can also allow giving the app index from command line. Not sure though how much time migrating 10000 apps(default value of max number of apps in store) in state store will take.
2. Current code will continue as it is. We can abort running of RM if we find mismatch in the scheme used for storing of apps. We can then warn the admin to run the tool above before he tries to restart RM.

We suffered from this problem too. Seems this issue has last for nearly half a year, any new progress guys?

Was waiting for an input from Vinod Kumar Vavilapalli and Arun Suresh so that we reach a common understanding on what we will do on the backward compatibility part.

Anyways in the coming week, plan to upload a patch implementing one of the approaches discussed.

YARN-3643 should help alleviate most of the issues users face. This JIRA could be targeted only at trunk, without worrying about rolling upgrades.

For a trunk only patch, do we really need to be concerned about backward compatibility here? The expectation when moving to 3.0.0 would be that none of the apps survive an upgrade.

Karthik Kambatla,
The thing we primarily need to decide is do we need to be concerned about configuration changes ? Or assume that state store will be formatted if this config is changed.
Code in patch will throw an exception if app node format is not as per configuration, so on recovery, exception will indicate state store has to be formatted.

Since we don't support rolling upgrades across major versions, it should be okay to require a state-store format.

Karthik Kambatla, I mean if let us say somebody configures the split index as 3 initially but later wants to change it to 2. In such a case we assume state store will have to be formatted ?

Oh, sorry.

For changing split index post upgrade to 3.x.y, it would be nice to make the change seamlessly. If that is not possible, requiring a format should be okay as long as we document it clearly.

Hi,

We're looking at switching to a HA RM and I'm a bit concerned about this ticket, since we have a very active RM. Couple questions for those who encountered the bug:

• how many applications did you have in the RM store before this became a problem?
• was switching the zk max messages size via -Djute.maxbuffer=<bytes> a viable workaround?

Also, is there a sense of how close this ticket is to being merged? Thanks,

Ben

how many applications did you have in the RM store before this became a problem

Will have to check. I think it was more than 10000 apps in our case. Will let you know.

switching the zk max messages size via -Djute.maxbuffer=<bytes> a viable workaround?

Yes, that works. Also we can set a lower config value for number of completed apps to be stored in state store. Even 0 can be set.

Also, is there a sense of how close this ticket is to being merged?

The patches currently here have to be rebased because of recent changes. Had put this on the back burner as this will go in trunk and not in branch-2. If its required to be handled earlier, I will focus on it. Plan to take this up in the coming month anyways.

Got it. Thanks for the details. It sounds like we'll have some workarounds available if we do run into trouble, which is hopefully good enough for now.

Rebased and updated the patch.
Additionally the patch ensures that changes in split index config does not lead to formatting of the state store.
The patch primarily adopts the suggestion given by Vinod Kumar Vavilapalli above.
The storage scheme would look something like below.

  |--- RM_APP_ROOT
  |     |----- HIERARCHIES
  |     |        |----- 1
  |     |        |      |----- (#ApplicationId barring last character)
  |     |        |      |       |----- (#Last character of ApplicationId)
  |     |        |      |       |       |----- (#ApplicationAttemptIds)
  |     |        |      ....
  |     |        |
  |     |        |----- 2
  |     |        |      |----- (#ApplicationId barring last 2 characters)
  |     |        |      |       |----- (#Last 2 characters of ApplicationId)
  |     |        |      |       |       |----- (#ApplicationAttemptIds)
  |     |        |      ....
  |     |        |
  |     |        |----- 3
  |     |        |      |----- (#ApplicationId barring last 3 characters)
  |     |        |      |       |----- (#Last 3 characters of ApplicationId)
  |     |        |      |       |       |----- (#ApplicationAttemptIds)
  |     |        |      ....
  |     |        |
  |     |        |----- 4
  |     |        |      |----- (#ApplicationId barring last 4 characters)
  |     |        |      |       |----- (#Last 4 characters of ApplicationId)
  |     |        |      |       |       |----- (#ApplicationAttemptIds)
  |     |        |      ....
  |     |        |
  |     |----- (#ApplicationId1)
  |     |        |----- (#ApplicationAttemptIds)
  |     |
  |     |----- (#ApplicationId2)
  |     |       |----- (#ApplicationAttemptIds)
  |     ....
  |

Split index will be calculated from the end.
Apps will be stored outside HIERARCHIES folder(i.e. directly under RMAppRoot) if the split index config value is 0(i.e. no split) - default value. This has been done so that if users do not want to split app nodes, there will be no impact on them during an upgrade.

If app node is not found as in the folder as per configured split index, we will look into other paths.
At the time of startup, we will include only those app hierarchies which have apps under them and the hierarchy as per configured split index. This would preclude the need to look in each and every path(as per split index) in case app znode is not found in path as per configured split index.

Example : With no split, appid znode will be of the stored as RMAppRoot/application_1352994193343_0001. If the value of this config is 1, the appid znode will be broken into two parts application_1352994193343_000 and 1 respectively with former being the parent node.
It will be stored in path RMAppRoot/HIERARCHIES/1/application_1352994193343_000/1 i.e. upto 10 apps can be stored under the parent.
If config was 2, it will be stored in path RMAppRoot/HIERARCHIES/2/application_1352994193343_00/01 i.e. upto 100 apps can be stored under the parent.
Likewise, upto 1000 apps can be stored under a parent if config is 3 and 10000 apps if config is 4.

We remove the parent app path if no apps exist under the parent upon removal.

As the ZKRMStateStore methods are synchronized I am assuming that there will be no race when deleting the parent path above. i.e. Race while deleting the app node parent and a new app being stored under the same parent. I hope that is a fair assumption assuming only one RM will be active at a time. And only one RM should be up in non HA mode.
Do we need to take care of something else here ?

We can decide if we need to write a tool to migrate apps to a different hierarchy(can be done in another JIRA) and whether we would want this in branch-2.

-1 overall

This message was automatically generated.

Karthik Kambatla, Jian He, Vinod Kumar Vavilapalli, Arun Suresh, kindly review.
Will address checkstyle in next patch.

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?

  static final int NO_APPID_NODE_SPLIT = 0;

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.

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

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.

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

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.

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

should be

    rmAppRootHierarchies = new HashMap<>(5);

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

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

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

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

Please include messages in your @throws comments.

  /**
   * 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);
     }
   }

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.

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

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.

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

should be

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

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.

Thanks Daniel for the review.

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.

That is done in loadRMAppState. Only those hierarchies which have apps are considered. We can go with not creating the directories which are not configured ever, at all. But I went with this approach.

what happens if an app happens to exist in more than one split?

Should not happen until and unless somebody does manual operations in state store. And in that case, a lot can go wrong with state store. So we do make some assumptions, even in previous code.
However, I am not sure whether we need to fence certain operations or not as I highlighted in an earlier comment. But could not come up with a scenario because I felt state store operations are synchronized, inconsistency should not occur.

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?

Ok...

I'm not sure this constant adds anything. I found it made the code harder to read than just hard-coding in 0.

Hmm...If its harder to read, I can put 0 everywhere. This value should not change in future.

This violates the Principle of Least Astonishment. At least log a warning that you're not doing what the user said to.

Correct, a warning log should be added.

I don't think the accessors are needed.

Yes, they are not required.

Might want to swap those method names.

Agree. safeDeleteIfExists makes more sense in the other case.

should be HashMap<ApplicationAttemptId, RMAppAttempt> attempts = new HashMap<>();

Yeah, <> can be used to reduce clutter.

Regarding other comments, will add more comments to make tests and main code more easier to read and fix missing javadocs.

Varun Saxena, apologize for the really long delay..

I am fine with the approach mentioned here. Will take a closer look at the patch shortly..

-1 overall

This message was automatically generated.

Thanks Arun Suresh.
Seems patch is not applying. Will upload a patch after rebasing to trunk and fixing Daniel's comments by tomorrow(as its late night here). Maybe you can have a look then.
It would be good if we can get this into 3.0.0-alpha because we were thinking of including this fix in our private code.

I think one part which mainly needed a discussion on, was how do we delete the parent application node(application is now split into 2 nodes) if it contains no children. This check is currently done when application is being removed. We are not having the whole operation under a single fencing as we have to check number children after deletion. If 2 RMs' can ever become active at same time, this can potentially lead to a race. Maybe we can just swallow NotEmptyException during deletion of parent.

-1 overall

This message was automatically generated.

Have rebased the patch and fixed comments given by Daniel. Haven't tested it in my setup after rebase but tests pass and should be good for review.

As per current patch, there can be a race if 2 RMs' become active at same time. Do we need to handle it because I have never come across this scenario.
Basically when we store an app, we first create parent app node as per split(if it does not exist) and then create the child znode which stores app data. These 2 operations are not carried out within the same fencing though.
And when we remove an app, we first delete the app znode containing app data, then get children for parent app node to check if there are no more child znodes, and if there are no more child znodes, delete the parent app node. These 3 operations are not done in a single fencing either which can lead to a race between creating and deleting parent app node.

We can however get rid of this potential race by doing these operations within a single fencing by creating fencing lock nodes explicitly first and then carrying out these operations one by one(cant be done in single transaction as we have to check how many children exist for the parent app node). Thoughts ?

Varun Saxena, can you see if the patch applies now, especially since we now have Curator

No it doesn't.
Will rebase it

Varun Saxena, any progress on the rebase?

Sorry have been keeping busy lately.
Give me a week. Will update it.

No worries. I'm happy to help with the review.

Varun Saxena, I went ahead and did the rebase for the latest patch. Can you take a look and make sure I didn't screw anything up?

-1 overall

This message was automatically generated.

Varun Saxena, ping.

Varun Saxena, can we get this one closed?

Sorry missed your pings. Will look at it in a couple of days.

Rebasing. Let's see what Jenkins says.

-1 overall

This message was automatically generated.

Sweet! Thanks for the rebase, Varun Saxena. It's been a while, so starting over with a fresh review. Lots of minor points, but no major issues.

1. In ZKRMStateStore.loadRMAppState(), I think leafNodePath should be parentNodePath to be clearer: String leafNodePath = getNodePath(appRoot, childNodeName);
2. In ZKRMStateStore.loadRMAppState(), the final if in the for shouldn't be performed if splitIndex is 0:

         if (splitIndex != appIdNodeSplitIndex && !appNodeFound) {
           // If no loaded app exists for a particular split index and the split
           // index for which apps are being loaded is not the one configured, then
           // we do not need to keep track of this hierarchy for storing/updating/
           // removing app/app attempt znodes.
           rmAppRootHierarchies.remove(splitIndex);
         }

   It doesn't hurt anything, though. Maybe best to just add a comment that says it's OK to remove something that doesn't exist?

3. In ZKRMStateStore.loadApplicationAttemptState(), the if (LOG.isDebugEnabled()) is superfluous. The arg to the log call doesn't cost anything to create.
4. ZKRMStateStore.checkRemoveParentAppNode() is missing the description for the @throws tag. Same in both getLeafAppIdNodePath() methods.
5. In ZKRMStateStore.checkRemoveParentAppNode() the last log line isn't wrapped in an if like all the others
6. In ZKRMStateStore.getLeafAppIdNodePath(), I'd prefer if we didn't do assignments to the parameters
7. In ZKRMStateStore.getLeafAppIdNodePath() the log line isn't wrapped in an if like all the others
8. This is maybe a bit pedantic, but shouldn't the exception in ZKRMStateStore.storeApplicationAttemptStateInternal() be a YarnException instead of a YarnRuntimeException? Unchecked exceptions should be unexpected. Failing to store an app in ZK is an obvious possibility.
9. Seems like the new logic in ZKRMStateStore.storeApplicationAttemptStateInternal() and ZKRMStateStore.updateApplicationAttemptStateInternal() should be refactored into another method maybe. You could also use it from removeApplicationAttemptInternal(), removeApplicationStateInternal(), and removeApplication()
10. In RMZKStateStore.getSplitAppNodeParent(), can we add a comment to explain why we're subtracting - 1 from the length - split index?
11. Instead of TestZKRMStateStore.createPath(), can we use a Guava Joiner?
12. appId isn't used in TestZKRMStateStore.verifyLoadedAttempt()
13. Super minor, but in TestZKRMStateStore.testAppNodeSplit(), it would be nice to visually separate the app2 code from the app1 code:

        // Store attempt associated with app1.
        Token<AMRMTokenIdentifier> appAttemptToken1 =
            generateAMRMToken(attemptId1, appTokenMgr);
        SecretKey clientTokenKey1 =
            clientToAMTokenMgr.createMasterKey(attemptId1);
        ContainerId containerId1 =
            ConverterUtils.toContainerId("container_1352994193343_0001_01_000001");
        storeAttempt(store, attemptId1, containerId1.toString(), appAttemptToken1,
            clientTokenKey1, dispatcher);
        String appAttemptIdStr2 = "appattempt_1352994193343_0001_000002";
        ApplicationAttemptId attemptId2 =
            ConverterUtils.toApplicationAttemptId(appAttemptIdStr2);
    
        // Store attempt associated with app2.
        Token<AMRMTokenIdentifier> appAttemptToken2 =
            generateAMRMToken(attemptId2, appTokenMgr);
        SecretKey clientTokenKey2 =
            clientToAMTokenMgr.createMasterKey(attemptId2);
        Credentials attemptCred = new Credentials();
        attemptCred.addSecretKey(RMStateStore.AM_CLIENT_TOKEN_MASTER_KEY_NAME,
            clientTokenKey2.getEncoded());
        ContainerId containerId2 =
            ConverterUtils.toContainerId("container_1352994193343_0001_02_000001");
        storeAttempt(store, attemptId2, containerId2.toString(), appAttemptToken2,
            clientTokenKey2, dispatcher);
    

    Note that the last two statements in the app1 section are actually for app2.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Fixed some checkstyle issues

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

+1 overall

This message was automatically generated.

Thanks for updating the patch, Varun Saxena. Looks like you addressed most of my comments. I took a look again at the ones you didn't address, and I'm fine with leaving them unaddressed. One minor quibble in the new patch: "deduct" is misspelled as "dedcut". Otherwise, LGTM.

+1 overall

This message was automatically generated.

Thanks, Varun Saxena. Let me do one more quick review of the whole thing, and then let's get it in.

A couple of quick questions:

1. Why are you removing the rm1.close() and rm2.close() in TestZKRMStateStore.testFencing()?
2. Why drop this log line LOG.info("Unknown child node with name: " + childNodeName); in loadRMAppState()

Also, two super minor quibbles:

  private static RMApp createMockAppForRemove(ApplicationId appId,
      ApplicationAttemptId...attemptIds) {

should have a space after the ellipses.

In yarn-default.xml, the first line should have a space before the parenthesis.

If you don't mind posting a patch to fix the spaces, that would be great. If not, I'll file a couple newbie JIRAs to fix them post-commit.

Thanks Daniel for the review.
I will update the patch.

Why are you removing the rm1.close() and rm2.close() in TestZKRMStateStore.testFencing()?

I am actually adding it.

Fixed other comments.
Sorry was on leave so could not update patch earlier.

-1 overall

This message was automatically generated.

+1 on the latest patch.

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11652 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11652/)
YARN-2962. ZKRMStateStore: Limit the number of znodes under a znode (templedf: rev 2e52789edf68016e7a3f450164f8bd3d8e6cb210)

• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/recovery/RMStateStoreTestBase.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/resources/yarn-default.xml
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/recovery/TestZKRMStateStore.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestRMStoreCommands.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/recovery/ZKRMStateStore.java

Thanks for all the patches, Varun Saxena! Thanks for the reviews and discussion, Karthik Kambatla, Arun Suresh, and Vinod Kumar Vavilapalli. Committed to trunk and branch-2.

Since it's a compatible change that doesn't mess with existing properties, I went ahead and pulled it into branch-2. I'm running on not a lot of sleep right now, though, so I'd feel better if someone double-checked me on the sensibility of that.

Thanks a lot Daniel Templeton for the review and commit. This JIRA was pending since a long time.

The attempt in the patch was to have seamless transition from one split index to another and/or from previous version to this one.
We look through alternate paths to decide where application is stored.

Thanks Karthik Kambatla and Arun Suresh for the reviews as well.
Thanks Rakesh R for your suggestion and insights into the behavior of Zookeeper.

Varun Saxena/Daniel Templeton

Had the time to check the change . Currently as per the following code since the VERSION is marked as 2 .

161	  protected static final Version CURRENT_VERSION_INFO = Version
162	      .newInstance(2, 0);

RMStateStore#checkVersion will fail and RM will not be able to start with old state store. We can keep the version as 1.3 itself

Thanks Bibin A Chundatt. You are correct. I forgot to discuss regarding not changing the major state store version.

To be honest we can raise a separate JIRA for this as well as it has been a fair while since this JIRA went in.
If everyone agrees we can just change the minor version. Otherwise, this will be an incompatible change. Barring state store version, this should be a compatible fix though.
The only issue is that node structure has changed quite a bit(although old nodes still have the same structure). Is it mandatory to change the major state store version if the node structure has changed this much?

cc Daniel Templeton, Karthik Kambatla, let me know your thoughts on this.

Varun Saxena
Raised YARN-6703 jira is for the same. Would you like to give patch for the same based on discussion