Repository: apache/hadoop
File: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/conf/ZKConfigurationStore.java (Java)

Code before:
   * Creates a new path in Zookeeper only, if it does not already exist.
   *
   * @param path Value of the Zookeeper path
   * @return <code>true</code>if the creation executed; <code>false</code>
   * otherwise.
   * @throws Exception
   */
  private boolean createNewZkPath(String path) throws Exception {
    if (!zkManager.exists(path)) {
      try {
        zkManager.create(path, zkAcl);
      } catch(NodeExistsException e) {
        LOG.warn(NODEEXISTS_MSG, e);
        return false;
      }
      return true;
    } else {
      return false;
    }
  }

  @VisibleForTesting
  protected byte[] getZkData(String path) throws Exception {
    // Should a 'yarn resourcemanager -format-state-store' command is issued
    // while one of the RM is in a starting state, there is a time period
    // when the /confstore/CONF_STORE path does not exist, on a different
    // occasion the confstore is being written and the getZkData method
    // returns a null value causing the RM to fail.
    // To prevent this, added a re-try mechanism before giving up.
    int maxRetries = 6;
    int attempt = 1;
    int sleepBetweenRetries = conf.getInt(
        YarnConfiguration.RM_SCHEDCONF_STORE_ZK_READ_RETRY_SECS,
        YarnConfiguration.DEFAULT_RM_SCHEDCONF_STORE_ZK_READ_RETRY_SECS);

    while (attempt < maxRetries) {
      if(zkManager.exists(path)) {
        LOG.debug("zkManager.exists(path) {} exists.", path);
        byte[] zkData = zkManager.getData(path);
        if (zkData != null && zkData.length > 0) {
          LOG.debug("We are returning the zkData OK!");
          return zkData;
        }
      }
      LOG.warn("The ZK CONFSTORE path or the ZkData was null. Retrying in {} "
              + "seconds... (Attempt {})", sleepBetweenRetries, attempt);
      TimeUnit.SECONDS.sleep(sleepBetweenRetries);
      attempt++;
    }
    LOG.error("The ZK CONFSTORE path or the ZkData was null. Giving up.");
    return new byte[0];

Review comment (bug):
I am not really clear of the phrase below:
> "on a different occasion the confstore is being written"

I would phrase it like the following: 

> while one of the RM is in a starting state, there is a time period
> when the /confstore/CONF_STORE path does not exist or the ZkData has not fully been written 
> then the getZkData method returns a null value causing the RM to fail.
> To prevent this, added a re-try mechanism before giving up.

Please let me know what do you think

Code after:
   * Creates a new path in Zookeeper only, if it does not already exist.
   *
   * @param path Value of the Zookeeper path
   * @return <code>true</code>if the creation executed; <code>false</code>
   * otherwise.
   * @throws Exception
   */
  private boolean createNewZkPath(String path) throws Exception {
    if (!zkManager.exists(path)) {
      try {
        zkManager.create(path, zkAcl);
      } catch(NodeExistsException e) {
        LOG.warn(NODEEXISTS_MSG, e);
        return false;
      }
      return true;
    } else {
      return false;
    }
  }

  @VisibleForTesting
  protected byte[] getZkData(String path) throws Exception {
    // If the 'yarn resourcemanager -format-conf-store' command is issued
    // while one of the RMs is in a starting state, the RM may fail. This
    // occurs because the /confstore/CONF_STORE path may not yet exist.
    // Alternatively, if the confstore is in the process of being written,
    // the getZkData method returns a null value, causing the crash.
    //To prevent this, added a re-try mechanism before giving up.
    int maxRetries = 6;
    int attempt = 1;
    int sleepBetweenRetries = conf.getInt(
        YarnConfiguration.RM_SCHEDCONF_STORE_ZK_READ_RETRY_SECS,
        YarnConfiguration.DEFAULT_RM_SCHEDCONF_STORE_ZK_READ_RETRY_SECS);

    while (attempt < maxRetries) {
      if(zkManager.exists(path)) {
        LOG.debug("zkManager.exists(path) {} exists.", path);
        byte[] zkData = zkManager.getData(path);
        if (zkData != null && zkData.length > 0) {
          LOG.debug("We are returning the zkData OK!");
          return zkData;
        }
      }
      LOG.warn("The ZK CONFSTORE path or the ZkData was null. Retrying in {} "
              + "seconds... (Attempt {})", sleepBetweenRetries, attempt);
      TimeUnit.SECONDS.sleep(sleepBetweenRetries);
      attempt++;
    }
    LOG.error("The ZK CONFSTORE path or the ZkData was null. Giving up.");
    return new byte[0];