Details

    • Type: Bug
    • Status: Resolved
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9.0, 3.0.0-alpha4
    • Component/s: None
    • Labels:
    • Hadoop Flags:
      Reviewed

      Description

            LOG.debug(appId + " znode didn't exist. Created a new znode to"
                + " update the application state.");
      

      Check is debug enabled

              if (LOG.isDebugEnabled()) {
                LOG.debug((isUpdate ? "Storing " : "Updating ")
                    + dtSequenceNumberPath + ". SequenceNumber: "
                    + rmDTIdentifier.getSequenceNumber());
              }
      

      isUpdate will be always false

      1. YARN-6103.001.patch
        4 kB
        Daniel Sturman
      2. YARN-6103.002.patch
        3 kB
        Daniel Sturman

        Activity

        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11189 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11189/)
        YARN-6103. Log updates for ZKRMStateStore (Contributed by Daniel Sturman (templedf: rev 87852b6ef4b9d973b7b3999974d41c8860fb1495)

        • (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 #11189 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11189/ ) YARN-6103 . Log updates for ZKRMStateStore (Contributed by Daniel Sturman (templedf: rev 87852b6ef4b9d973b7b3999974d41c8860fb1495) (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 -

        Committed to trunk and branch-2. Thanks, Daniel Sturman and Bibin A Chundatt!

        Show
        templedf Daniel Templeton added a comment - Committed to trunk and branch-2. Thanks, Daniel Sturman and Bibin A Chundatt !
        Hide
        templedf Daniel Templeton added a comment -

        Thanks for the review, Bibin A Chundatt. Thanks for the patch, Daniel Sturman. Committing now...

        Show
        templedf Daniel Templeton added a comment - Thanks for the review, Bibin A Chundatt . Thanks for the patch, Daniel Sturman . Committing now...
        Hide
        bibinchundatt Bibin A Chundatt added a comment -

        +1 Latest patch looks good to me too

        Show
        bibinchundatt Bibin A Chundatt added a comment - +1 Latest patch looks good to me too
        Hide
        templedf Daniel Templeton added a comment -

        Thanks for the update. Latest patch looks good to me. The test failure is unrelated. +1

        I'll commit it later in the week to give Bibin A Chundatt a chance to comment if he wants.

        Show
        templedf Daniel Templeton added a comment - Thanks for the update. Latest patch looks good to me. The test failure is unrelated. +1 I'll commit it later in the week to give Bibin A Chundatt a chance to comment if he wants.
        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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
        +1 mvninstall 12m 52s trunk passed
        +1 compile 0m 31s trunk passed
        +1 checkstyle 0m 20s trunk passed
        +1 mvnsite 0m 33s trunk passed
        +1 mvneclipse 0m 14s trunk passed
        +1 findbugs 0m 59s trunk passed
        +1 javadoc 0m 20s trunk passed
        +1 mvninstall 0m 30s the patch passed
        +1 compile 0m 28s the patch passed
        +1 javac 0m 28s the patch passed
        +1 checkstyle 0m 18s the patch passed
        +1 mvnsite 0m 31s the patch passed
        +1 mvneclipse 0m 12s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 3s the patch passed
        +1 javadoc 0m 18s the patch passed
        -1 unit 40m 33s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 17s The patch does not generate ASF License warnings.
        61m 30s



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



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-6103
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12850025/YARN-6103.002.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 401c2ba4d93b 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 312b36d
        Default Java 1.8.0_121
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-YARN-Build/14791/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/14791/testReport/
        modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
        Console output https://builds.apache.org/job/PreCommit-YARN-Build/14791/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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 12m 52s trunk passed +1 compile 0m 31s trunk passed +1 checkstyle 0m 20s trunk passed +1 mvnsite 0m 33s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 0m 59s trunk passed +1 javadoc 0m 20s trunk passed +1 mvninstall 0m 30s the patch passed +1 compile 0m 28s the patch passed +1 javac 0m 28s the patch passed +1 checkstyle 0m 18s the patch passed +1 mvnsite 0m 31s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 3s the patch passed +1 javadoc 0m 18s the patch passed -1 unit 40m 33s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 61m 30s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-6103 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12850025/YARN-6103.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 401c2ba4d93b 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 312b36d Default Java 1.8.0_121 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-YARN-Build/14791/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/14791/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/14791/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 -

        Pretty much. If debug-level logging is not enabled, log.debug(...) is a no-op. If the argument to it is expensive to construct, that work is done and thrown away.

        Show
        templedf Daniel Templeton added a comment - Pretty much. If debug-level logging is not enabled, log.debug(...) is a no-op. If the argument to it is expensive to construct, that work is done and thrown away.
        Hide
        sturman Daniel Sturman added a comment -

        Just to clarify, you're saying its not possible for a log level to be set at Debug without isDebugEnabled() being true? So we just use the latter as a shortcut to avoid expensive string construction?

        Show
        sturman Daniel Sturman added a comment - Just to clarify, you're saying its not possible for a log level to be set at Debug without isDebugEnabled() being true? So we just use the latter as a shortcut to avoid expensive string construction?
        Hide
        templedf Daniel Templeton added a comment -

        Thanks for the patch, Daniel Sturman!

        I think your read of the code is correct; the storing/updating thing was backwards.

        The check around the debug line in logRootNodeAcls() is superfluous because the check is always done before calling the method. In general we only do the check before doing something expensive, so the one at the end of loadApplicationAttemptState(), for example, is also unnecessary.

        While you're at it, I'd pull the preceding log line into the if since it's the same info as the one in the else. That way we don't get (more or less) the same message twice when creating a new entry.

        Show
        templedf Daniel Templeton added a comment - Thanks for the patch, Daniel Sturman ! I think your read of the code is correct; the storing/updating thing was backwards. The check around the debug line in logRootNodeAcls() is superfluous because the check is always done before calling the method. In general we only do the check before doing something expensive, so the one at the end of loadApplicationAttemptState() , for example, is also unnecessary. While you're at it, I'd pull the preceding log line into the if since it's the same info as the one in the else . That way we don't get (more or less) the same message twice when creating a new entry.
        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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
        +1 mvninstall 15m 12s trunk passed
        +1 compile 0m 39s trunk passed
        +1 checkstyle 0m 27s trunk passed
        +1 mvnsite 0m 40s trunk passed
        +1 mvneclipse 0m 18s trunk passed
        +1 findbugs 1m 14s trunk passed
        +1 javadoc 0m 28s trunk passed
        +1 mvninstall 0m 39s the patch passed
        +1 compile 0m 36s the patch passed
        +1 javac 0m 36s the patch passed
        +1 checkstyle 0m 21s the patch passed
        +1 mvnsite 0m 37s the patch passed
        +1 mvneclipse 0m 14s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 22s the patch passed
        +1 javadoc 0m 23s the patch passed
        +1 unit 42m 38s hadoop-yarn-server-resourcemanager in the patch passed.
        +1 asflicense 0m 18s The patch does not generate ASF License warnings.
        67m 45s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-6103
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12849910/YARN-6103.001.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux de1ff5b305e2 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 / 312b36d
        Default Java 1.8.0_121
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/14785/testReport/
        modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
        Console output https://builds.apache.org/job/PreCommit-YARN-Build/14785/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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 15m 12s trunk passed +1 compile 0m 39s trunk passed +1 checkstyle 0m 27s trunk passed +1 mvnsite 0m 40s trunk passed +1 mvneclipse 0m 18s trunk passed +1 findbugs 1m 14s trunk passed +1 javadoc 0m 28s trunk passed +1 mvninstall 0m 39s the patch passed +1 compile 0m 36s the patch passed +1 javac 0m 36s the patch passed +1 checkstyle 0m 21s the patch passed +1 mvnsite 0m 37s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 22s the patch passed +1 javadoc 0m 23s the patch passed +1 unit 42m 38s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 67m 45s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-6103 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12849910/YARN-6103.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux de1ff5b305e2 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 / 312b36d Default Java 1.8.0_121 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/14785/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/14785/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        sturman Daniel Sturman added a comment -

        This patch assumes I was understanding things per my comment correctly. If not, reply to the comment and I'll revise.

        Show
        sturman Daniel Sturman added a comment - This patch assumes I was understanding things per my comment correctly. If not, reply to the comment and I'll revise.
        Hide
        sturman Daniel Sturman added a comment -

        With isUpdate clearly always being false... Is that debug message even correct?
        When isUpdate is false shouldn't it be logging the action as "Storing" vs "Updating"? Having it be false and then reporting Updating seems strange (not fully knowing this code). I think the correct debug here would be:

        if (LOG.isDebugEnabled()) {
                  LOG.debug("Storing "
                      + dtSequenceNumberPath + ". SequenceNumber: "
                      + rmDTIdentifier.getSequenceNumber());
                }
        
        Show
        sturman Daniel Sturman added a comment - With isUpdate clearly always being false... Is that debug message even correct? When isUpdate is false shouldn't it be logging the action as "Storing" vs "Updating"? Having it be false and then reporting Updating seems strange (not fully knowing this code). I think the correct debug here would be: if (LOG.isDebugEnabled()) { LOG.debug( "Storing " + dtSequenceNumberPath + ". SequenceNumber: " + rmDTIdentifier.getSequenceNumber()); }

          People

          • Assignee:
            sturman Daniel Sturman
            Reporter:
            bibinchundatt Bibin A Chundatt
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development