Uploaded image for project: 'Hadoop YARN'
  1. Hadoop YARN
  2. YARN-2962

ZKRMStateStore: Limit the number of znodes under a znode

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.6.0
    • Fix Version/s: 2.9.0, 3.0.0-alpha4
    • Component/s: resourcemanager
    • Labels:
      None

      Description

      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.

      1. YARN-2962.01.patch
        32 kB
        Varun Saxena
      2. YARN-2962.2.patch
        31 kB
        Arun Suresh
      3. YARN-2962.3.patch
        32 kB
        Arun Suresh
      4. YARN-2962.04.patch
        58 kB
        Varun Saxena
      5. YARN-2962.05.patch
        63 kB
        Varun Saxena
      6. YARN-2962.006.patch
        62 kB
        Daniel Templeton
      7. YARN-2962.007.patch
        62 kB
        Varun Saxena
      8. YARN-2962.008.patch
        66 kB
        Varun Saxena
      9. YARN-2962.008.patch
        66 kB
        Varun Saxena
      10. YARN-2962.009.patch
        65 kB
        Varun Saxena
      11. YARN-2962.010.patch
        65 kB
        Varun Saxena
      12. YARN-2962.011.patch
        65 kB
        Varun Saxena

        Issue Links

          Activity

          Hide
          bibinchundatt Bibin A Chundatt added a comment - - edited

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

          Show
          bibinchundatt Bibin A Chundatt added a comment - - edited Varun Saxena Raised YARN-6703 jira is for the same. Would you like to give patch for the same based on discussion
          Hide
          varun_saxena Varun Saxena added a comment -

          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.

          Show
          varun_saxena Varun Saxena added a comment - 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.
          Hide
          bibinchundatt Bibin A Chundatt added a comment - - edited

          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

          Show
          bibinchundatt Bibin A Chundatt added a comment - - edited 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
          Hide
          varun_saxena Varun Saxena added a comment - - edited

          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.

          Show
          varun_saxena Varun Saxena added a comment - - edited 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.
          Hide
          templedf Daniel Templeton added a comment -

          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.

          Show
          templedf Daniel Templeton added a comment - 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.
          Hide
          templedf Daniel Templeton added a comment -

          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.

          Show
          templedf Daniel Templeton added a comment - 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.
          Hide
          hudson Hudson added a comment -

          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
          Show
          hudson Hudson added a comment - 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
          Hide
          templedf Daniel Templeton added a comment -

          +1 on the latest patch.

          Show
          templedf Daniel Templeton added a comment - +1 on the latest patch.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 16s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 0m 38s Maven dependency ordering for branch
          +1 mvninstall 14m 5s trunk passed
          +1 compile 10m 33s trunk passed
          +1 checkstyle 0m 53s trunk passed
          +1 mvnsite 1m 56s trunk passed
          +1 mvneclipse 1m 3s trunk passed
          -1 findbugs 1m 6s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common in trunk has 2 extant Findbugs warnings.
          -1 findbugs 1m 15s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager in trunk has 8 extant Findbugs warnings.
          +1 javadoc 1m 29s trunk passed
          0 mvndep 0m 9s Maven dependency ordering for patch
          +1 mvninstall 1m 41s the patch passed
          +1 compile 9m 16s the patch passed
          +1 javac 9m 16s the patch passed
          -0 checkstyle 0m 53s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 250 unchanged - 2 fixed = 255 total (was 252)
          +1 mvnsite 1m 54s the patch passed
          +1 mvneclipse 0m 59s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 3m 47s the patch passed
          +1 javadoc 1m 24s the patch passed
          +1 unit 0m 32s hadoop-yarn-api in the patch passed.
          +1 unit 2m 24s hadoop-yarn-common in the patch passed.
          -1 unit 39m 25s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 37s The patch does not generate ASF License warnings.
          106m 1s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart
            hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:0ac17dc
          JIRA Issue YARN-2962
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12865198/YARN-2962.011.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 4eb1202abcba 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 8b5f2c3
          Default Java 1.8.0_121
          findbugs v3.1.0-RC1
          findbugs https://builds.apache.org/job/PreCommit-YARN-Build/15751/artifact/patchprocess/branch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common-warnings.html
          findbugs https://builds.apache.org/job/PreCommit-YARN-Build/15751/artifact/patchprocess/branch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-warnings.html
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15751/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/15751/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15751/testReport/
          modules C: 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 U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/15751/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 0m 38s Maven dependency ordering for branch +1 mvninstall 14m 5s trunk passed +1 compile 10m 33s trunk passed +1 checkstyle 0m 53s trunk passed +1 mvnsite 1m 56s trunk passed +1 mvneclipse 1m 3s trunk passed -1 findbugs 1m 6s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common in trunk has 2 extant Findbugs warnings. -1 findbugs 1m 15s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager in trunk has 8 extant Findbugs warnings. +1 javadoc 1m 29s trunk passed 0 mvndep 0m 9s Maven dependency ordering for patch +1 mvninstall 1m 41s the patch passed +1 compile 9m 16s the patch passed +1 javac 9m 16s the patch passed -0 checkstyle 0m 53s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 250 unchanged - 2 fixed = 255 total (was 252) +1 mvnsite 1m 54s the patch passed +1 mvneclipse 0m 59s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 3m 47s the patch passed +1 javadoc 1m 24s the patch passed +1 unit 0m 32s hadoop-yarn-api in the patch passed. +1 unit 2m 24s hadoop-yarn-common in the patch passed. -1 unit 39m 25s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 37s The patch does not generate ASF License warnings. 106m 1s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart   hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer Subsystem Report/Notes Docker Image:yetus/hadoop:0ac17dc JIRA Issue YARN-2962 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12865198/YARN-2962.011.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 4eb1202abcba 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 8b5f2c3 Default Java 1.8.0_121 findbugs v3.1.0-RC1 findbugs https://builds.apache.org/job/PreCommit-YARN-Build/15751/artifact/patchprocess/branch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common-warnings.html findbugs https://builds.apache.org/job/PreCommit-YARN-Build/15751/artifact/patchprocess/branch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-warnings.html checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15751/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/15751/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15751/testReport/ modules C: 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 U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/15751/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          varun_saxena Varun Saxena added a comment -

          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.

          Show
          varun_saxena Varun Saxena added a comment - 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.
          Hide
          varun_saxena Varun Saxena added a comment -

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

          Show
          varun_saxena Varun Saxena added a comment - Thanks Daniel for the review. I will update the patch.
          Hide
          templedf Daniel Templeton added a comment -

          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.

          Show
          templedf Daniel Templeton added a comment - A couple of quick questions: Why are you removing the rm1.close() and rm2.close() in TestZKRMStateStore.testFencing() ? 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.
          Hide
          templedf Daniel Templeton added a comment -

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

          Show
          templedf Daniel Templeton added a comment - Thanks, Varun Saxena . Let me do one more quick review of the whole thing, and then let's get it in.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 27s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 0m 10s Maven dependency ordering for branch
          +1 mvninstall 14m 28s trunk passed
          +1 compile 11m 1s trunk passed
          +1 checkstyle 0m 58s trunk passed
          +1 mvnsite 1m 56s trunk passed
          +1 mvneclipse 1m 3s trunk passed
          +1 findbugs 3m 38s trunk passed
          +1 javadoc 1m 34s trunk passed
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 29s the patch passed
          +1 compile 9m 39s the patch passed
          +1 javac 9m 39s the patch passed
          -0 checkstyle 0m 56s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 250 unchanged - 2 fixed = 255 total (was 252)
          +1 mvnsite 1m 54s the patch passed
          +1 mvneclipse 1m 0s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 4m 14s the patch passed
          +1 javadoc 1m 42s the patch passed
          +1 unit 0m 40s hadoop-yarn-api in the patch passed.
          +1 unit 2m 44s hadoop-yarn-common in the patch passed.
          +1 unit 40m 29s hadoop-yarn-server-resourcemanager in the patch passed.
          +1 asflicense 0m 34s The patch does not generate ASF License warnings.
          109m 55s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue YARN-2962
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12861637/YARN-2962.010.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 48c6a2bc9e2a 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / a4b5aa8
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15479/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15479/testReport/
          modules C: 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 U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/15479/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 27s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 0m 10s Maven dependency ordering for branch +1 mvninstall 14m 28s trunk passed +1 compile 11m 1s trunk passed +1 checkstyle 0m 58s trunk passed +1 mvnsite 1m 56s trunk passed +1 mvneclipse 1m 3s trunk passed +1 findbugs 3m 38s trunk passed +1 javadoc 1m 34s trunk passed 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 29s the patch passed +1 compile 9m 39s the patch passed +1 javac 9m 39s the patch passed -0 checkstyle 0m 56s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 250 unchanged - 2 fixed = 255 total (was 252) +1 mvnsite 1m 54s the patch passed +1 mvneclipse 1m 0s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 4m 14s the patch passed +1 javadoc 1m 42s the patch passed +1 unit 0m 40s hadoop-yarn-api in the patch passed. +1 unit 2m 44s hadoop-yarn-common in the patch passed. +1 unit 40m 29s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 34s The patch does not generate ASF License warnings. 109m 55s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-2962 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12861637/YARN-2962.010.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 48c6a2bc9e2a 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / a4b5aa8 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15479/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15479/testReport/ modules C: 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 U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/15479/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          templedf Daniel Templeton added a comment -

          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.

          Show
          templedf Daniel Templeton added a comment - 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.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 29s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 0m 53s Maven dependency ordering for branch
          +1 mvninstall 15m 10s trunk passed
          +1 compile 7m 11s trunk passed
          +1 checkstyle 1m 8s trunk passed
          +1 mvnsite 2m 32s trunk passed
          +1 mvneclipse 1m 18s trunk passed
          +1 findbugs 4m 48s trunk passed
          +1 javadoc 1m 48s trunk passed
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 32s the patch passed
          +1 compile 6m 33s the patch passed
          +1 javac 6m 33s the patch passed
          -0 checkstyle 1m 2s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 250 unchanged - 2 fixed = 255 total (was 252)
          +1 mvnsite 2m 10s the patch passed
          +1 mvneclipse 1m 18s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 4m 19s the patch passed
          +1 javadoc 1m 48s the patch passed
          +1 unit 0m 41s hadoop-yarn-api in the patch passed.
          +1 unit 2m 43s hadoop-yarn-common in the patch passed.
          +1 unit 41m 24s hadoop-yarn-server-resourcemanager in the patch passed.
          +1 asflicense 0m 49s The patch does not generate ASF License warnings.
          109m 36s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue YARN-2962
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859042/YARN-2962.009.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux af4ac52c9b80 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 6d95866
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15296/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15296/testReport/
          modules C: 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 U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/15296/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 29s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 0m 53s Maven dependency ordering for branch +1 mvninstall 15m 10s trunk passed +1 compile 7m 11s trunk passed +1 checkstyle 1m 8s trunk passed +1 mvnsite 2m 32s trunk passed +1 mvneclipse 1m 18s trunk passed +1 findbugs 4m 48s trunk passed +1 javadoc 1m 48s trunk passed 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 32s the patch passed +1 compile 6m 33s the patch passed +1 javac 6m 33s the patch passed -0 checkstyle 1m 2s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 250 unchanged - 2 fixed = 255 total (was 252) +1 mvnsite 2m 10s the patch passed +1 mvneclipse 1m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 4m 19s the patch passed +1 javadoc 1m 48s the patch passed +1 unit 0m 41s hadoop-yarn-api in the patch passed. +1 unit 2m 43s hadoop-yarn-common in the patch passed. +1 unit 41m 24s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 49s The patch does not generate ASF License warnings. 109m 36s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-2962 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859042/YARN-2962.009.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux af4ac52c9b80 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 6d95866 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15296/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15296/testReport/ modules C: 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 U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/15296/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 24s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 0m 45s Maven dependency ordering for branch
          +1 mvninstall 12m 34s trunk passed
          +1 compile 6m 4s trunk passed
          +1 checkstyle 1m 1s trunk passed
          +1 mvnsite 2m 3s trunk passed
          +1 mvneclipse 1m 13s trunk passed
          +1 findbugs 3m 36s trunk passed
          +1 javadoc 1m 40s trunk passed
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 21s the patch passed
          +1 compile 5m 38s the patch passed
          +1 javac 5m 38s the patch passed
          -0 checkstyle 0m 59s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 250 unchanged - 2 fixed = 255 total (was 252)
          +1 mvnsite 2m 1s the patch passed
          +1 mvneclipse 1m 11s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 3m 55s the patch passed
          +1 javadoc 1m 39s the patch passed
          +1 unit 0m 37s hadoop-yarn-api in the patch passed.
          +1 unit 2m 33s hadoop-yarn-common in the patch passed.
          -1 unit 40m 34s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 42s The patch does not generate ASF License warnings.
          100m 0s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue YARN-2962
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859042/YARN-2962.009.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 56f62c645868 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 6d95866
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15295/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/15295/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15295/testReport/
          modules C: 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 U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/15295/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 24s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 0m 45s Maven dependency ordering for branch +1 mvninstall 12m 34s trunk passed +1 compile 6m 4s trunk passed +1 checkstyle 1m 1s trunk passed +1 mvnsite 2m 3s trunk passed +1 mvneclipse 1m 13s trunk passed +1 findbugs 3m 36s trunk passed +1 javadoc 1m 40s trunk passed 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 21s the patch passed +1 compile 5m 38s the patch passed +1 javac 5m 38s the patch passed -0 checkstyle 0m 59s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 250 unchanged - 2 fixed = 255 total (was 252) +1 mvnsite 2m 1s the patch passed +1 mvneclipse 1m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 3m 55s the patch passed +1 javadoc 1m 39s the patch passed +1 unit 0m 37s hadoop-yarn-api in the patch passed. +1 unit 2m 33s hadoop-yarn-common in the patch passed. -1 unit 40m 34s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 42s The patch does not generate ASF License warnings. 100m 0s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-2962 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859042/YARN-2962.009.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 56f62c645868 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 6d95866 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15295/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/15295/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15295/testReport/ modules C: 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 U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/15295/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 18s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 0m 8s Maven dependency ordering for branch
          +1 mvninstall 12m 43s trunk passed
          +1 compile 6m 31s trunk passed
          +1 checkstyle 0m 58s trunk passed
          +1 mvnsite 2m 4s trunk passed
          +1 mvneclipse 1m 7s trunk passed
          +1 findbugs 4m 5s trunk passed
          +1 javadoc 1m 38s trunk passed
          0 mvndep 0m 9s Maven dependency ordering for patch
          +1 mvninstall 1m 38s the patch passed
          +1 compile 6m 23s the patch passed
          +1 javac 6m 23s the patch passed
          -0 checkstyle 0m 59s hadoop-yarn-project/hadoop-yarn: The patch generated 6 new + 250 unchanged - 2 fixed = 256 total (was 252)
          +1 mvnsite 2m 0s the patch passed
          +1 mvneclipse 1m 11s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 3m 55s the patch passed
          +1 javadoc 1m 38s the patch passed
          +1 unit 0m 37s hadoop-yarn-api in the patch passed.
          +1 unit 2m 31s hadoop-yarn-common in the patch passed.
          -1 unit 40m 1s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 38s The patch does not generate ASF License warnings.
          100m 22s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue YARN-2962
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12858957/YARN-2962.009.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 10b8bd9cda70 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 615ac09
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15293/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/15293/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15293/testReport/
          modules C: 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 U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/15293/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 18s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 0m 8s Maven dependency ordering for branch +1 mvninstall 12m 43s trunk passed +1 compile 6m 31s trunk passed +1 checkstyle 0m 58s trunk passed +1 mvnsite 2m 4s trunk passed +1 mvneclipse 1m 7s trunk passed +1 findbugs 4m 5s trunk passed +1 javadoc 1m 38s trunk passed 0 mvndep 0m 9s Maven dependency ordering for patch +1 mvninstall 1m 38s the patch passed +1 compile 6m 23s the patch passed +1 javac 6m 23s the patch passed -0 checkstyle 0m 59s hadoop-yarn-project/hadoop-yarn: The patch generated 6 new + 250 unchanged - 2 fixed = 256 total (was 252) +1 mvnsite 2m 0s the patch passed +1 mvneclipse 1m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 3m 55s the patch passed +1 javadoc 1m 38s the patch passed +1 unit 0m 37s hadoop-yarn-api in the patch passed. +1 unit 2m 31s hadoop-yarn-common in the patch passed. -1 unit 40m 1s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 38s The patch does not generate ASF License warnings. 100m 22s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-2962 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12858957/YARN-2962.009.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 10b8bd9cda70 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 615ac09 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15293/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/15293/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15293/testReport/ modules C: 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 U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/15293/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          varun_saxena Varun Saxena added a comment -

          Fixed some checkstyle issues

          Show
          varun_saxena Varun Saxena added a comment - Fixed some checkstyle issues
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
          +1 @author 0m 1s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 0m 9s Maven dependency ordering for branch
          +1 mvninstall 13m 8s trunk passed
          -1 compile 4m 52s hadoop-yarn in trunk failed.
          +1 checkstyle 0m 59s trunk passed
          +1 mvnsite 2m 1s trunk passed
          +1 mvneclipse 1m 12s trunk passed
          +1 findbugs 3m 30s trunk passed
          +1 javadoc 1m 40s trunk passed
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 20s the patch passed
          -1 compile 4m 38s hadoop-yarn in the patch failed.
          -1 javac 4m 38s hadoop-yarn in the patch failed.
          -0 checkstyle 0m 57s hadoop-yarn-project/hadoop-yarn: The patch generated 6 new + 252 unchanged - 0 fixed = 258 total (was 252)
          +1 mvnsite 2m 0s the patch passed
          +1 mvneclipse 1m 8s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 3m 56s the patch passed
          +1 javadoc 1m 43s the patch passed
          +1 unit 0m 39s hadoop-yarn-api in the patch passed.
          +1 unit 2m 54s hadoop-yarn-common in the patch passed.
          +1 unit 39m 55s hadoop-yarn-server-resourcemanager in the patch passed.
          +1 asflicense 0m 41s The patch does not generate ASF License warnings.
          97m 4s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue YARN-2962
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12858656/YARN-2962.008.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux d305a38eae8a 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 34424e9
          Default Java 1.8.0_121
          compile https://builds.apache.org/job/PreCommit-YARN-Build/15263/artifact/patchprocess/branch-compile-hadoop-yarn-project_hadoop-yarn.txt
          findbugs v3.0.0
          compile https://builds.apache.org/job/PreCommit-YARN-Build/15263/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn.txt
          javac https://builds.apache.org/job/PreCommit-YARN-Build/15263/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15263/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15263/testReport/
          modules C: 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 U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/15263/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated. +1 @author 0m 1s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 0m 9s Maven dependency ordering for branch +1 mvninstall 13m 8s trunk passed -1 compile 4m 52s hadoop-yarn in trunk failed. +1 checkstyle 0m 59s trunk passed +1 mvnsite 2m 1s trunk passed +1 mvneclipse 1m 12s trunk passed +1 findbugs 3m 30s trunk passed +1 javadoc 1m 40s trunk passed 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 20s the patch passed -1 compile 4m 38s hadoop-yarn in the patch failed. -1 javac 4m 38s hadoop-yarn in the patch failed. -0 checkstyle 0m 57s hadoop-yarn-project/hadoop-yarn: The patch generated 6 new + 252 unchanged - 0 fixed = 258 total (was 252) +1 mvnsite 2m 0s the patch passed +1 mvneclipse 1m 8s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 3m 56s the patch passed +1 javadoc 1m 43s the patch passed +1 unit 0m 39s hadoop-yarn-api in the patch passed. +1 unit 2m 54s hadoop-yarn-common in the patch passed. +1 unit 39m 55s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 41s The patch does not generate ASF License warnings. 97m 4s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-2962 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12858656/YARN-2962.008.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux d305a38eae8a 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 34424e9 Default Java 1.8.0_121 compile https://builds.apache.org/job/PreCommit-YARN-Build/15263/artifact/patchprocess/branch-compile-hadoop-yarn-project_hadoop-yarn.txt findbugs v3.0.0 compile https://builds.apache.org/job/PreCommit-YARN-Build/15263/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn.txt javac https://builds.apache.org/job/PreCommit-YARN-Build/15263/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15263/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15263/testReport/ modules C: 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 U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/15263/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 23s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 0m 55s Maven dependency ordering for branch
          +1 mvninstall 14m 57s trunk passed
          -1 compile 5m 26s hadoop-yarn in trunk failed.
          +1 checkstyle 0m 58s trunk passed
          +1 mvnsite 2m 2s trunk passed
          +1 mvneclipse 1m 13s trunk passed
          +1 findbugs 3m 30s trunk passed
          +1 javadoc 1m 41s trunk passed
          0 mvndep 0m 9s Maven dependency ordering for patch
          +1 mvninstall 1m 20s the patch passed
          -1 compile 4m 21s hadoop-yarn in the patch failed.
          -1 javac 4m 21s hadoop-yarn in the patch failed.
          -0 checkstyle 0m 57s hadoop-yarn-project/hadoop-yarn: The patch generated 6 new + 252 unchanged - 0 fixed = 258 total (was 252)
          +1 mvnsite 1m 58s the patch passed
          +1 mvneclipse 1m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          -1 findbugs 1m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
          +1 javadoc 1m 38s the patch passed
          +1 unit 0m 36s hadoop-yarn-api in the patch passed.
          +1 unit 2m 31s hadoop-yarn-common in the patch passed.
          -1 unit 40m 8s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 47s The patch does not generate ASF License warnings.
          99m 50s



          Reason Tests
          FindBugs module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
            Switch statement found in org.apache.hadoop.yarn.server.resourcemanager.recovery.ZKRMStateStore.handleApplicationAttemptStateOp(ApplicationAttemptId, ApplicationAttemptStateData, ZKRMStateStore$AppAttemptOp) where one case falls through to the next case At ZKRMStateStore.java:ApplicationAttemptStateData, ZKRMStateStore$AppAttemptOp) where one case falls through to the next case At ZKRMStateStore.java:[lines 828-833]
          Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue YARN-2962
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12858540/YARN-2962.008.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 08f28cd069f4 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 5a40baf
          Default Java 1.8.0_121
          compile https://builds.apache.org/job/PreCommit-YARN-Build/15247/artifact/patchprocess/branch-compile-hadoop-yarn-project_hadoop-yarn.txt
          findbugs v3.0.0
          compile https://builds.apache.org/job/PreCommit-YARN-Build/15247/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn.txt
          javac https://builds.apache.org/job/PreCommit-YARN-Build/15247/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15247/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          findbugs https://builds.apache.org/job/PreCommit-YARN-Build/15247/artifact/patchprocess/new-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.html
          unit https://builds.apache.org/job/PreCommit-YARN-Build/15247/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15247/testReport/
          modules C: 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 U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/15247/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 23s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 0m 55s Maven dependency ordering for branch +1 mvninstall 14m 57s trunk passed -1 compile 5m 26s hadoop-yarn in trunk failed. +1 checkstyle 0m 58s trunk passed +1 mvnsite 2m 2s trunk passed +1 mvneclipse 1m 13s trunk passed +1 findbugs 3m 30s trunk passed +1 javadoc 1m 41s trunk passed 0 mvndep 0m 9s Maven dependency ordering for patch +1 mvninstall 1m 20s the patch passed -1 compile 4m 21s hadoop-yarn in the patch failed. -1 javac 4m 21s hadoop-yarn in the patch failed. -0 checkstyle 0m 57s hadoop-yarn-project/hadoop-yarn: The patch generated 6 new + 252 unchanged - 0 fixed = 258 total (was 252) +1 mvnsite 1m 58s the patch passed +1 mvneclipse 1m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. -1 findbugs 1m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) +1 javadoc 1m 38s the patch passed +1 unit 0m 36s hadoop-yarn-api in the patch passed. +1 unit 2m 31s hadoop-yarn-common in the patch passed. -1 unit 40m 8s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 47s The patch does not generate ASF License warnings. 99m 50s Reason Tests FindBugs module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager   Switch statement found in org.apache.hadoop.yarn.server.resourcemanager.recovery.ZKRMStateStore.handleApplicationAttemptStateOp(ApplicationAttemptId, ApplicationAttemptStateData, ZKRMStateStore$AppAttemptOp) where one case falls through to the next case At ZKRMStateStore.java:ApplicationAttemptStateData, ZKRMStateStore$AppAttemptOp) where one case falls through to the next case At ZKRMStateStore.java: [lines 828-833] Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-2962 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12858540/YARN-2962.008.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 08f28cd069f4 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 5a40baf Default Java 1.8.0_121 compile https://builds.apache.org/job/PreCommit-YARN-Build/15247/artifact/patchprocess/branch-compile-hadoop-yarn-project_hadoop-yarn.txt findbugs v3.0.0 compile https://builds.apache.org/job/PreCommit-YARN-Build/15247/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn.txt javac https://builds.apache.org/job/PreCommit-YARN-Build/15247/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15247/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt findbugs https://builds.apache.org/job/PreCommit-YARN-Build/15247/artifact/patchprocess/new-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.html unit https://builds.apache.org/job/PreCommit-YARN-Build/15247/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15247/testReport/ modules C: 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 U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/15247/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          templedf Daniel Templeton added a comment -

          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.

          Show
          templedf Daniel Templeton added a comment - 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. In ZKRMStateStore.loadRMAppState() , I think leafNodePath should be parentNodePath to be clearer: String leafNodePath = getNodePath(appRoot, childNodeName); 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? In ZKRMStateStore.loadApplicationAttemptState() , the if (LOG.isDebugEnabled()) is superfluous. The arg to the log call doesn't cost anything to create. ZKRMStateStore.checkRemoveParentAppNode() is missing the description for the @throws tag. Same in both getLeafAppIdNodePath() methods. In ZKRMStateStore.checkRemoveParentAppNode() the last log line isn't wrapped in an if like all the others In ZKRMStateStore.getLeafAppIdNodePath() , I'd prefer if we didn't do assignments to the parameters In ZKRMStateStore.getLeafAppIdNodePath() the log line isn't wrapped in an if like all the others 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. 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() In RMZKStateStore.getSplitAppNodeParent() , can we add a comment to explain why we're subtracting - 1 from the length - split index? Instead of TestZKRMStateStore.createPath() , can we use a Guava Joiner ? appId isn't used in TestZKRMStateStore.verifyLoadedAttempt() 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.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 19s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          0 mvndep 0m 45s Maven dependency ordering for branch
          +1 mvninstall 14m 8s trunk passed
          +1 compile 8m 51s trunk passed
          +1 checkstyle 0m 53s trunk passed
          +1 mvnsite 1m 58s trunk passed
          +1 mvneclipse 0m 58s trunk passed
          +1 findbugs 3m 46s trunk passed
          +1 javadoc 1m 30s trunk passed
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 34s the patch passed
          +1 compile 6m 14s the patch passed
          -1 javac 6m 14s hadoop-yarn-project_hadoop-yarn generated 15 new + 39 unchanged - 0 fixed = 54 total (was 39)
          +1 checkstyle 0m 54s the patch passed
          +1 mvnsite 1m 52s the patch passed
          +1 mvneclipse 0m 52s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 3m 59s the patch passed
          +1 javadoc 1m 24s the patch passed
          +1 unit 0m 31s hadoop-yarn-api in the patch passed.
          +1 unit 2m 40s hadoop-yarn-common in the patch passed.
          -1 unit 40m 46s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 29s The patch does not generate ASF License warnings.
          103m 18s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer
            hadoop.yarn.server.resourcemanager.scheduler.fair.TestFairScheduler
            hadoop.yarn.server.resourcemanager.TestRMStoreCommands
            hadoop.yarn.server.resourcemanager.TestOpportunisticContainerAllocatorAMService



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue YARN-2962
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12854929/YARN-2962.007.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux f1075a57b897 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 5f5b031
          Default Java 1.8.0_121
          findbugs v3.0.0
          javac https://builds.apache.org/job/PreCommit-YARN-Build/15096/artifact/patchprocess/diff-compile-javac-hadoop-yarn-project_hadoop-yarn.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/15096/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15096/testReport/
          modules C: 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 U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/15096/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 19s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. 0 mvndep 0m 45s Maven dependency ordering for branch +1 mvninstall 14m 8s trunk passed +1 compile 8m 51s trunk passed +1 checkstyle 0m 53s trunk passed +1 mvnsite 1m 58s trunk passed +1 mvneclipse 0m 58s trunk passed +1 findbugs 3m 46s trunk passed +1 javadoc 1m 30s trunk passed 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 34s the patch passed +1 compile 6m 14s the patch passed -1 javac 6m 14s hadoop-yarn-project_hadoop-yarn generated 15 new + 39 unchanged - 0 fixed = 54 total (was 39) +1 checkstyle 0m 54s the patch passed +1 mvnsite 1m 52s the patch passed +1 mvneclipse 0m 52s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 3m 59s the patch passed +1 javadoc 1m 24s the patch passed +1 unit 0m 31s hadoop-yarn-api in the patch passed. +1 unit 2m 40s hadoop-yarn-common in the patch passed. -1 unit 40m 46s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 29s The patch does not generate ASF License warnings. 103m 18s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer   hadoop.yarn.server.resourcemanager.scheduler.fair.TestFairScheduler   hadoop.yarn.server.resourcemanager.TestRMStoreCommands   hadoop.yarn.server.resourcemanager.TestOpportunisticContainerAllocatorAMService Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-2962 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12854929/YARN-2962.007.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux f1075a57b897 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 5f5b031 Default Java 1.8.0_121 findbugs v3.0.0 javac https://builds.apache.org/job/PreCommit-YARN-Build/15096/artifact/patchprocess/diff-compile-javac-hadoop-yarn-project_hadoop-yarn.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/15096/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15096/testReport/ modules C: 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 U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/15096/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          varun_saxena Varun Saxena added a comment -

          Rebasing. Let's see what Jenkins says.

          Show
          varun_saxena Varun Saxena added a comment - Rebasing. Let's see what Jenkins says.
          Hide
          varun_saxena Varun Saxena added a comment -

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

          Show
          varun_saxena Varun Saxena added a comment - Sorry missed your pings. Will look at it in a couple of days.
          Hide
          templedf Daniel Templeton added a comment -

          Varun Saxena, can we get this one closed?

          Show
          templedf Daniel Templeton added a comment - Varun Saxena , can we get this one closed?
          Hide
          templedf Daniel Templeton added a comment -

          Varun Saxena, ping.

          Show
          templedf Daniel Templeton added a comment - Varun Saxena , ping.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          0 mvndep 0m 10s Maven dependency ordering for branch
          +1 mvninstall 12m 42s trunk passed
          +1 compile 4m 58s trunk passed
          +1 checkstyle 0m 45s trunk passed
          +1 mvnsite 1m 42s trunk passed
          +1 mvneclipse 0m 57s trunk passed
          +1 findbugs 3m 12s trunk passed
          +1 javadoc 1m 23s trunk passed
          0 mvndep 0m 9s Maven dependency ordering for patch
          +1 mvninstall 1m 20s the patch passed
          +1 compile 4m 35s the patch passed
          -1 javac 4m 35s hadoop-yarn-project_hadoop-yarn generated 15 new + 35 unchanged - 0 fixed = 50 total (was 35)
          -0 checkstyle 0m 45s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 251 unchanged - 0 fixed = 256 total (was 251)
          +1 mvnsite 1m 41s the patch passed
          +1 mvneclipse 0m 54s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 3m 30s the patch passed
          +1 javadoc 1m 20s the patch passed
          +1 unit 0m 30s hadoop-yarn-api in the patch passed.
          +1 unit 2m 24s hadoop-yarn-common in the patch passed.
          -1 unit 48m 51s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 47s The patch does not generate ASF License warnings.
          101m 16s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.ahs.TestRMApplicationHistoryWriter
            hadoop.yarn.server.resourcemanager.TestRMStoreCommands



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue YARN-2962
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12846634/YARN-2962.006.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 314d0dfd68c2 3.13.0-103-generic #150-Ubuntu SMP Thu Nov 24 10:34:17 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / e692316
          Default Java 1.8.0_111
          findbugs v3.0.0
          javac https://builds.apache.org/job/PreCommit-YARN-Build/14628/artifact/patchprocess/diff-compile-javac-hadoop-yarn-project_hadoop-yarn.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/14628/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/14628/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/14628/testReport/
          modules C: 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 U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/14628/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. 0 mvndep 0m 10s Maven dependency ordering for branch +1 mvninstall 12m 42s trunk passed +1 compile 4m 58s trunk passed +1 checkstyle 0m 45s trunk passed +1 mvnsite 1m 42s trunk passed +1 mvneclipse 0m 57s trunk passed +1 findbugs 3m 12s trunk passed +1 javadoc 1m 23s trunk passed 0 mvndep 0m 9s Maven dependency ordering for patch +1 mvninstall 1m 20s the patch passed +1 compile 4m 35s the patch passed -1 javac 4m 35s hadoop-yarn-project_hadoop-yarn generated 15 new + 35 unchanged - 0 fixed = 50 total (was 35) -0 checkstyle 0m 45s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 251 unchanged - 0 fixed = 256 total (was 251) +1 mvnsite 1m 41s the patch passed +1 mvneclipse 0m 54s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 3m 30s the patch passed +1 javadoc 1m 20s the patch passed +1 unit 0m 30s hadoop-yarn-api in the patch passed. +1 unit 2m 24s hadoop-yarn-common in the patch passed. -1 unit 48m 51s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 47s The patch does not generate ASF License warnings. 101m 16s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.ahs.TestRMApplicationHistoryWriter   hadoop.yarn.server.resourcemanager.TestRMStoreCommands Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-2962 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12846634/YARN-2962.006.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 314d0dfd68c2 3.13.0-103-generic #150-Ubuntu SMP Thu Nov 24 10:34:17 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / e692316 Default Java 1.8.0_111 findbugs v3.0.0 javac https://builds.apache.org/job/PreCommit-YARN-Build/14628/artifact/patchprocess/diff-compile-javac-hadoop-yarn-project_hadoop-yarn.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/14628/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/14628/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/14628/testReport/ modules C: 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 U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/14628/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          templedf Daniel Templeton added a comment -

          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?

          Show
          templedf Daniel Templeton added a comment - 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?
          Hide
          templedf Daniel Templeton added a comment -

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

          Show
          templedf Daniel Templeton added a comment - No worries. I'm happy to help with the review.
          Hide
          varun_saxena Varun Saxena added a comment -

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

          Show
          varun_saxena Varun Saxena added a comment - Sorry have been keeping busy lately. Give me a week. Will update it.
          Hide
          templedf Daniel Templeton added a comment -

          Varun Saxena, any progress on the rebase?

          Show
          templedf Daniel Templeton added a comment - Varun Saxena , any progress on the rebase?
          Hide
          varun_saxena Varun Saxena added a comment -

          No it doesn't.
          Will rebase it

          Show
          varun_saxena Varun Saxena added a comment - No it doesn't. Will rebase it
          Hide
          asuresh Arun Suresh added a comment -

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

          Show
          asuresh Arun Suresh added a comment - Varun Saxena , can you see if the patch applies now, especially since we now have Curator
          Hide
          varun_saxena Varun Saxena added a comment -

          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 ?

          Show
          varun_saxena Varun Saxena added a comment - 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 ?
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 29s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          0 mvndep 0m 8s Maven dependency ordering for branch
          +1 mvninstall 7m 31s trunk passed
          +1 compile 2m 17s trunk passed
          +1 checkstyle 0m 40s trunk passed
          +1 mvnsite 1m 38s trunk passed
          +1 mvneclipse 0m 35s trunk passed
          +1 findbugs 3m 9s trunk passed
          +1 javadoc 1m 13s trunk passed
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 23s the patch passed
          +1 compile 2m 24s the patch passed
          +1 javac 2m 24s the patch passed
          -1 checkstyle 0m 39s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 259 unchanged - 2 fixed = 264 total (was 261)
          +1 mvnsite 1m 37s the patch passed
          +1 mvneclipse 0m 32s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 3m 26s the patch passed
          +1 javadoc 1m 6s the patch passed
          +1 unit 0m 25s hadoop-yarn-api in the patch passed.
          +1 unit 2m 21s hadoop-yarn-common in the patch passed.
          -1 unit 36m 32s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 16s The patch does not generate ASF License warnings.
          69m 28s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.TestClientRMTokens
            hadoop.yarn.server.resourcemanager.TestAMAuthorization



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:2c91fd8
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12808983/YARN-2962.05.patch
          JIRA Issue YARN-2962
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux a0be6640fa6d 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 5a43583
          Default Java 1.8.0_91
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/11919/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/11919/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/11919/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/11919/testReport/
          modules C: 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 U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/11919/console
          Powered by Apache Yetus 0.3.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 29s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. 0 mvndep 0m 8s Maven dependency ordering for branch +1 mvninstall 7m 31s trunk passed +1 compile 2m 17s trunk passed +1 checkstyle 0m 40s trunk passed +1 mvnsite 1m 38s trunk passed +1 mvneclipse 0m 35s trunk passed +1 findbugs 3m 9s trunk passed +1 javadoc 1m 13s trunk passed 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 23s the patch passed +1 compile 2m 24s the patch passed +1 javac 2m 24s the patch passed -1 checkstyle 0m 39s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 259 unchanged - 2 fixed = 264 total (was 261) +1 mvnsite 1m 37s the patch passed +1 mvneclipse 0m 32s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 3m 26s the patch passed +1 javadoc 1m 6s the patch passed +1 unit 0m 25s hadoop-yarn-api in the patch passed. +1 unit 2m 21s hadoop-yarn-common in the patch passed. -1 unit 36m 32s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 69m 28s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestClientRMTokens   hadoop.yarn.server.resourcemanager.TestAMAuthorization Subsystem Report/Notes Docker Image:yetus/hadoop:2c91fd8 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12808983/YARN-2962.05.patch JIRA Issue YARN-2962 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux a0be6640fa6d 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 5a43583 Default Java 1.8.0_91 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/11919/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/11919/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/11919/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/11919/testReport/ modules C: 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 U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/11919/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          varun_saxena Varun Saxena added a comment -

          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.

          Show
          varun_saxena Varun Saxena added a comment - 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.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          -1 patch 0m 4s YARN-2962 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help.



          Subsystem Report/Notes
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12780789/YARN-2962.04.patch
          JIRA Issue YARN-2962
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/11881/console
          Powered by Apache Yetus 0.3.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. -1 patch 0m 4s YARN-2962 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. Subsystem Report/Notes JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12780789/YARN-2962.04.patch JIRA Issue YARN-2962 Console output https://builds.apache.org/job/PreCommit-YARN-Build/11881/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          asuresh Arun Suresh added a comment -

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

          Show
          asuresh Arun Suresh added a comment - 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..
          Hide
          varun_saxena Varun Saxena added a comment -

          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.

          Show
          varun_saxena Varun Saxena added a comment - 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.
          Hide
          varun_saxena Varun Saxena added a comment -

          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.

          Show
          varun_saxena Varun Saxena added a comment - 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.
          Hide
          templedf Daniel Templeton added a comment -

          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.

          Show
          templedf Daniel Templeton added a comment - 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.
          Hide
          varun_saxena Varun Saxena added a comment -

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

          Show
          varun_saxena Varun Saxena added a comment - Karthik Kambatla , Jian He , Vinod Kumar Vavilapalli , Arun Suresh , kindly review. Will address checkstyle in next patch.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 7m 51s trunk passed
          +1 compile 2m 9s trunk passed with JDK v1.8.0_66
          +1 compile 2m 9s trunk passed with JDK v1.7.0_91
          +1 checkstyle 0m 29s trunk passed
          +1 mvnsite 1m 37s trunk passed
          +1 mvneclipse 0m 41s trunk passed
          +1 findbugs 3m 48s trunk passed
          +1 javadoc 1m 30s trunk passed with JDK v1.8.0_66
          +1 javadoc 3m 50s trunk passed with JDK v1.7.0_91
          +1 mvninstall 1m 22s the patch passed
          +1 compile 2m 7s the patch passed with JDK v1.8.0_66
          +1 javac 2m 7s the patch passed
          +1 compile 2m 7s the patch passed with JDK v1.7.0_91
          +1 javac 2m 7s the patch passed
          -1 checkstyle 0m 29s Patch generated 2 new checkstyle issues in hadoop-yarn-project/hadoop-yarn (total was 221, now 221).
          +1 mvnsite 1m 33s the patch passed
          +1 mvneclipse 0m 35s the patch passed
          +1 whitespace 0m 0s Patch has no whitespace issues.
          +1 xml 0m 0s The patch has no ill-formed XML file.
          +1 findbugs 4m 6s the patch passed
          +1 javadoc 1m 25s the patch passed with JDK v1.8.0_66
          +1 javadoc 3m 38s the patch passed with JDK v1.7.0_91
          +1 unit 0m 22s hadoop-yarn-api in the patch passed with JDK v1.8.0_66.
          +1 unit 1m 57s hadoop-yarn-common in the patch passed with JDK v1.8.0_66.
          -1 unit 65m 25s hadoop-yarn-server-resourcemanager in the patch failed with JDK v1.8.0_66.
          +1 unit 0m 23s hadoop-yarn-api in the patch passed with JDK v1.7.0_91.
          +1 unit 2m 10s hadoop-yarn-common in the patch passed with JDK v1.7.0_91.
          -1 unit 66m 39s hadoop-yarn-server-resourcemanager in the patch failed with JDK v1.7.0_91.
          +1 asflicense 0m 21s Patch does not generate ASF License warnings.
          180m 18s



          Reason Tests
          JDK v1.8.0_66 Failed junit tests hadoop.yarn.server.resourcemanager.TestClientRMTokens
            hadoop.yarn.server.resourcemanager.TestAMAuthorization
          JDK v1.7.0_91 Failed junit tests hadoop.yarn.server.resourcemanager.TestClientRMTokens
            hadoop.yarn.server.resourcemanager.TestAMAuthorization



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:0ca8df7
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12780789/YARN-2962.04.patch
          JIRA Issue YARN-2962
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux e983a74baa5d 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / c1462a6
          Default Java 1.7.0_91
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_66 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_91
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/10175/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/10175/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdk1.8.0_66.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/10175/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdk1.7.0_91.txt
          unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/10175/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-YARN-Build/10175/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdk1.7.0_91.txt
          JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/10175/testReport/
          modules C: 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 U: hadoop-yarn-project/hadoop-yarn
          Max memory used 76MB
          Powered by Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/10175/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 7m 51s trunk passed +1 compile 2m 9s trunk passed with JDK v1.8.0_66 +1 compile 2m 9s trunk passed with JDK v1.7.0_91 +1 checkstyle 0m 29s trunk passed +1 mvnsite 1m 37s trunk passed +1 mvneclipse 0m 41s trunk passed +1 findbugs 3m 48s trunk passed +1 javadoc 1m 30s trunk passed with JDK v1.8.0_66 +1 javadoc 3m 50s trunk passed with JDK v1.7.0_91 +1 mvninstall 1m 22s the patch passed +1 compile 2m 7s the patch passed with JDK v1.8.0_66 +1 javac 2m 7s the patch passed +1 compile 2m 7s the patch passed with JDK v1.7.0_91 +1 javac 2m 7s the patch passed -1 checkstyle 0m 29s Patch generated 2 new checkstyle issues in hadoop-yarn-project/hadoop-yarn (total was 221, now 221). +1 mvnsite 1m 33s the patch passed +1 mvneclipse 0m 35s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 xml 0m 0s The patch has no ill-formed XML file. +1 findbugs 4m 6s the patch passed +1 javadoc 1m 25s the patch passed with JDK v1.8.0_66 +1 javadoc 3m 38s the patch passed with JDK v1.7.0_91 +1 unit 0m 22s hadoop-yarn-api in the patch passed with JDK v1.8.0_66. +1 unit 1m 57s hadoop-yarn-common in the patch passed with JDK v1.8.0_66. -1 unit 65m 25s hadoop-yarn-server-resourcemanager in the patch failed with JDK v1.8.0_66. +1 unit 0m 23s hadoop-yarn-api in the patch passed with JDK v1.7.0_91. +1 unit 2m 10s hadoop-yarn-common in the patch passed with JDK v1.7.0_91. -1 unit 66m 39s hadoop-yarn-server-resourcemanager in the patch failed with JDK v1.7.0_91. +1 asflicense 0m 21s Patch does not generate ASF License warnings. 180m 18s Reason Tests JDK v1.8.0_66 Failed junit tests hadoop.yarn.server.resourcemanager.TestClientRMTokens   hadoop.yarn.server.resourcemanager.TestAMAuthorization JDK v1.7.0_91 Failed junit tests hadoop.yarn.server.resourcemanager.TestClientRMTokens   hadoop.yarn.server.resourcemanager.TestAMAuthorization Subsystem Report/Notes Docker Image:yetus/hadoop:0ca8df7 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12780789/YARN-2962.04.patch JIRA Issue YARN-2962 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux e983a74baa5d 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / c1462a6 Default Java 1.7.0_91 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_66 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_91 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/10175/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/10175/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdk1.8.0_66.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/10175/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdk1.7.0_91.txt unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/10175/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-YARN-Build/10175/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdk1.7.0_91.txt JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/10175/testReport/ modules C: 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 U: hadoop-yarn-project/hadoop-yarn Max memory used 76MB Powered by Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org Console output https://builds.apache.org/job/PreCommit-YARN-Build/10175/console This message was automatically generated.
          Hide
          varun_saxena Varun Saxena added a comment -

          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.

          Show
          varun_saxena Varun Saxena added a comment - 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.
          Hide
          varun_saxena Varun Saxena added a comment -

          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 ?

          Show
          varun_saxena Varun Saxena added a comment - 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 ?
          Hide
          bpodgursky Ben Podgursky added a comment -

          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.

          Show
          bpodgursky Ben Podgursky added a comment - 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.
          Hide
          varun_saxena Varun Saxena added a comment -

          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.

          Show
          varun_saxena Varun Saxena added a comment - 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.
          Hide
          bpodgursky Ben Podgursky added a comment -

          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

          Show
          bpodgursky Ben Podgursky added a comment - 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
          Hide
          kasha Karthik Kambatla added a comment -

          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.

          Show
          kasha Karthik Kambatla added a comment - 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.
          Hide
          varun_saxena Varun Saxena added a comment -

          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 ?

          Show
          varun_saxena Varun Saxena added a comment - 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 ?
          Hide
          kasha Karthik Kambatla added a comment -

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

          Show
          kasha Karthik Kambatla added a comment - Since we don't support rolling upgrades across major versions, it should be okay to require a state-store format.
          Hide
          varun_saxena Varun Saxena added a comment -

          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.

          Show
          varun_saxena Varun Saxena added a comment - 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.
          Hide
          kasha Karthik Kambatla added a comment -

          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.

          Show
          kasha Karthik Kambatla added a comment - 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.
          Hide
          kasha Karthik Kambatla added a comment -

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

          Show
          kasha Karthik Kambatla added a comment - YARN-3643 should help alleviate most of the issues users face. This JIRA could be targeted only at trunk, without worrying about rolling upgrades.
          Hide
          varun_saxena Varun Saxena added a comment -

          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.

          Show
          varun_saxena Varun Saxena added a comment - 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.
          Hide
          shadyxu Jun Xu added a comment -

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

          Show
          shadyxu Jun Xu added a comment - We suffered from this problem too. Seems this issue has last for nearly half a year, any new progress guys?
          Hide
          varun_saxena Varun Saxena added a comment -

          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.

          Show
          varun_saxena Varun Saxena added a comment - 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.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          Canceling the patch for continuing the discussion..

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - Canceling the patch for continuing the discussion..
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          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.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - 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.
          Hide
          asuresh Arun Suresh added a comment -

          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.

          Show
          asuresh Arun Suresh added a comment - 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.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          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.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - 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.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 14m 35s Pre-patch trunk compilation is healthy.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 tests included 0m 0s The patch appears to include 1 new or modified test files.
          +1 whitespace 0m 0s The patch has no lines that end in whitespace.
          +1 javac 7m 33s There were no new javac warning messages.
          +1 javadoc 9m 34s There were no new javadoc warning messages.
          +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings.
          -1 checkstyle 5m 23s The applied patch generated 2 additional checkstyle issues.
          +1 install 1m 32s mvn install still works.
          +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
          +1 findbugs 3m 58s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          +1 yarn tests 0m 25s Tests passed in hadoop-yarn-api.
          +1 yarn tests 2m 0s Tests passed in hadoop-yarn-common.
          +1 yarn tests 52m 35s Tests passed in hadoop-yarn-server-resourcemanager.
              98m 33s  



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12728835/YARN-2962.3.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 99fe03e
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7524/artifact/patchprocess/checkstyle-result-diff.txt
          hadoop-yarn-api test log https://builds.apache.org/job/PreCommit-YARN-Build/7524/artifact/patchprocess/testrun_hadoop-yarn-api.txt
          hadoop-yarn-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7524/artifact/patchprocess/testrun_hadoop-yarn-common.txt
          hadoop-yarn-server-resourcemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7524/artifact/patchprocess/testrun_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7524/testReport/
          Java 1.7.0_55
          uname Linux asf905.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/7524/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 35s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 1 new or modified test files. +1 whitespace 0m 0s The patch has no lines that end in whitespace. +1 javac 7m 33s There were no new javac warning messages. +1 javadoc 9m 34s There were no new javadoc warning messages. +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 5m 23s The applied patch generated 2 additional checkstyle issues. +1 install 1m 32s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 3m 58s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 yarn tests 0m 25s Tests passed in hadoop-yarn-api. +1 yarn tests 2m 0s Tests passed in hadoop-yarn-common. +1 yarn tests 52m 35s Tests passed in hadoop-yarn-server-resourcemanager.     98m 33s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12728835/YARN-2962.3.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 99fe03e checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7524/artifact/patchprocess/checkstyle-result-diff.txt hadoop-yarn-api test log https://builds.apache.org/job/PreCommit-YARN-Build/7524/artifact/patchprocess/testrun_hadoop-yarn-api.txt hadoop-yarn-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7524/artifact/patchprocess/testrun_hadoop-yarn-common.txt hadoop-yarn-server-resourcemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7524/artifact/patchprocess/testrun_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7524/testReport/ Java 1.7.0_55 uname Linux asf905.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-YARN-Build/7524/console This message was automatically generated.
          Hide
          asuresh Arun Suresh added a comment -

          Updating with a few minor fixes.

          Show
          asuresh Arun Suresh added a comment - Updating with a few minor fixes.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 14m 33s Pre-patch trunk compilation is healthy.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 tests included 0m 0s The patch appears to include 1 new or modified test files.
          -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace.
          +1 javac 7m 29s There were no new javac warning messages.
          +1 javadoc 9m 33s There were no new javadoc warning messages.
          +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings.
          -1 checkstyle 5m 26s The applied patch generated 2 additional checkstyle issues.
          +1 install 1m 34s mvn install still works.
          +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
          +1 findbugs 4m 0s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          +1 yarn tests 0m 22s Tests passed in hadoop-yarn-api.
          +1 yarn tests 1m 56s Tests passed in hadoop-yarn-common.
          +1 yarn tests 56m 3s Tests passed in hadoop-yarn-server-resourcemanager.
              101m 54s  



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12728726/YARN-2962.2.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 99fe03e
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/7519/artifact/patchprocess/whitespace.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7519/artifact/patchprocess/checkstyle-result-diff.txt
          hadoop-yarn-api test log https://builds.apache.org/job/PreCommit-YARN-Build/7519/artifact/patchprocess/testrun_hadoop-yarn-api.txt
          hadoop-yarn-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7519/artifact/patchprocess/testrun_hadoop-yarn-common.txt
          hadoop-yarn-server-resourcemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7519/artifact/patchprocess/testrun_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7519/testReport/
          Java 1.7.0_55
          uname Linux asf906.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/7519/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 33s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 1 new or modified test files. -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. +1 javac 7m 29s There were no new javac warning messages. +1 javadoc 9m 33s There were no new javadoc warning messages. +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 5m 26s The applied patch generated 2 additional checkstyle issues. +1 install 1m 34s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 4m 0s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 yarn tests 0m 22s Tests passed in hadoop-yarn-api. +1 yarn tests 1m 56s Tests passed in hadoop-yarn-common. +1 yarn tests 56m 3s Tests passed in hadoop-yarn-server-resourcemanager.     101m 54s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12728726/YARN-2962.2.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 99fe03e whitespace https://builds.apache.org/job/PreCommit-YARN-Build/7519/artifact/patchprocess/whitespace.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7519/artifact/patchprocess/checkstyle-result-diff.txt hadoop-yarn-api test log https://builds.apache.org/job/PreCommit-YARN-Build/7519/artifact/patchprocess/testrun_hadoop-yarn-api.txt hadoop-yarn-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7519/artifact/patchprocess/testrun_hadoop-yarn-common.txt hadoop-yarn-server-resourcemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7519/artifact/patchprocess/testrun_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7519/testReport/ Java 1.7.0_55 uname Linux asf906.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-YARN-Build/7519/console This message was automatically generated.
          Hide
          asuresh Arun Suresh added a comment -

          Updating Patch:

          • Rebased with trunk
          • As discussed in the comments, the split index is now evaluated from the end of the appId.
          Show
          asuresh Arun Suresh added a comment - Updating Patch: Rebased with trunk As discussed in the comments, the split index is now evaluated from the end of the appId.
          Hide
          varun_saxena Varun Saxena added a comment -

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

          Show
          varun_saxena Varun Saxena added a comment - A little busy nowadays. The change I suggested though wont take much time. Will try to complete over the weekend and update a patch.
          Hide
          asuresh Arun Suresh added a comment -

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

          Show
          asuresh Arun Suresh added a comment - Varun Saxena , wondering if you need any help with this. Would like to get this in soon.
          Hide
          asuresh Arun Suresh added a comment -

          .. alternative to starting the index from the front.

          Show
          asuresh Arun Suresh added a comment - .. alternative to starting the index from the front.
          Hide
          asuresh Arun Suresh added a comment -

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

          Show
          asuresh Arun Suresh added a comment - Yup.. agreed, star index from the end is a better
          Hide
          varun_saxena Varun Saxena added a comment -

          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.

          Show
          varun_saxena Varun Saxena added a comment - 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.
          Hide
          varun_saxena Varun Saxena added a comment -

          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.

          Show
          varun_saxena Varun Saxena added a comment - 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.
          Hide
          asuresh Arun Suresh added a comment -

          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)

          Show
          asuresh Arun Suresh added a comment - Varun Saxena , Thanks for the patch. It looks pretty good.. also like the extensive testing. Couple of minor comments: 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 ? minor indentation errors (think you are using tabs) in loadRMAppState() and removeApplicationStateInternal() 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)
          Hide
          varun_saxena Varun Saxena added a comment -

          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 ?

          Show
          varun_saxena Varun Saxena added a comment - 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 ?
          Hide
          varun_saxena Varun Saxena added a comment -
          Show
          varun_saxena Varun Saxena added a comment - Karthik Kambatla / Jian He / Tsuyoshi Ozawa , kindly review
          Hide
          varun_saxena Varun Saxena added a comment -

          Findbugs to be addressed by YARN-3204

          Show
          varun_saxena Varun Saxena added a comment - Findbugs to be addressed by YARN-3204
          Hide
          hadoopqa Hadoop QA added a comment -

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

          Show
          hadoopqa Hadoop QA added a comment - -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.
          Hide
          kasha Karthik Kambatla added a comment -

          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.

          Show
          kasha Karthik Kambatla added a comment - 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.
          Hide
          ozawa Tsuyoshi Ozawa added a comment -

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

          Show
          ozawa Tsuyoshi Ozawa added a comment - Varun Saxena , Yes, RMStateStore#checkVersion validates the version. I think this change can be a major upgrade of the version.
          Hide
          varun_saxena Varun Saxena added a comment -

          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.

          Show
          varun_saxena Varun Saxena added a comment - 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.
          Hide
          varun_saxena Varun Saxena added a comment -

          Yup, that sounds good.

          Show
          varun_saxena Varun Saxena added a comment - Yup, that sounds good.
          Hide
          kasha Karthik Kambatla added a comment -

          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.

          Show
          kasha Karthik Kambatla added a comment - 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.
          Hide
          varun_saxena Varun Saxena added a comment -

          Karthik Kambatla, your views on this

          Show
          varun_saxena Varun Saxena added a comment - Karthik Kambatla , your views on this
          Hide
          varun_saxena Varun Saxena added a comment -

          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.

          Show
          varun_saxena Varun Saxena added a comment - 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.
          Hide
          rakeshr Rakesh R added a comment -

          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

          Show
          rakeshr Rakesh R added a comment - 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
          Hide
          kasha Karthik Kambatla added a comment -

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

          Show
          kasha Karthik Kambatla added a comment - Rakesh R - you are right, we saw the second case.
          Hide
          rakeshr Rakesh R added a comment -

          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!

          Show
          rakeshr Rakesh R added a comment - 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: znode data size - user added large data on a znode and submitting request to the ZooKeeper server. 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!

            People

            • Assignee:
              varun_saxena Varun Saxena
              Reporter:
              kasha Karthik Kambatla
            • Votes:
              0 Vote for this issue
              Watchers:
              30 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development