Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: HDFS-1312
    • Fix Version/s: 3.0.0-alpha2
    • Component/s: balancer & mover
    • Labels:
      None

      Description

      In HDFS-9671, Arpit Agarwal commented that we can possibly reduce the code path that is holding a lock while shutting down the diskBalancer. This jira tracks that improvement.

      1. HDFS-9849.001.patch
        1 kB
        Yuanbo Liu
      2. HDFS-9849.002.patch
        2 kB
        Yuanbo Liu
      3. HDFS-9849.003.patch
        2 kB
        Yuanbo Liu

        Activity

        Hide
        yuanbo Yuanbo Liu added a comment -

        Anu Engineer I'm testing disk-balancer and find this jira. If you don't mind, I'd like to provide a patch for this improvement, seems that this jira is easy for me to fix.

        Show
        yuanbo Yuanbo Liu added a comment - Anu Engineer I'm testing disk-balancer and find this jira. If you don't mind, I'd like to provide a patch for this improvement, seems that this jira is easy for me to fix.
        Hide
        anu Anu Engineer added a comment -

        sure, please go ahead.

        Show
        anu Anu Engineer added a comment - sure, please go ahead.
        Hide
        yuanbo Yuanbo Liu added a comment -

        Anu Engineer Thanks very much! uploaded v1 patch for this jira.

        Show
        yuanbo Yuanbo Liu added a comment - Anu Engineer Thanks very much! uploaded v1 patch for this jira.
        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 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 7m 13s trunk passed
        +1 compile 0m 44s trunk passed
        +1 checkstyle 0m 25s trunk passed
        +1 mvnsite 0m 51s trunk passed
        +1 mvneclipse 0m 12s trunk passed
        +1 findbugs 1m 41s trunk passed
        +1 javadoc 0m 53s trunk passed
        +1 mvninstall 0m 46s the patch passed
        +1 compile 0m 41s the patch passed
        +1 javac 0m 41s the patch passed
        +1 checkstyle 0m 22s the patch passed
        +1 mvnsite 0m 48s the patch passed
        +1 mvneclipse 0m 9s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        -1 findbugs 1m 48s hadoop-hdfs-project/hadoop-hdfs generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0)
        +1 javadoc 0m 51s the patch passed
        +1 unit 57m 21s hadoop-hdfs in the patch passed.
        +1 asflicense 0m 19s The patch does not generate ASF License warnings.
        76m 37s



        Reason Tests
        FindBugs module:hadoop-hdfs-project/hadoop-hdfs
          org.apache.hadoop.hdfs.server.datanode.DiskBalancer.cancelPlan(String) does not release lock on all paths At DiskBalancer.java:on all paths At DiskBalancer.java:[line 233]
          org.apache.hadoop.hdfs.server.datanode.DiskBalancer.shutdown() does not release lock on all paths At DiskBalancer.java:on all paths At DiskBalancer.java:[line 123]



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Issue HDFS-9849
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12827120/HDFS-9849.001.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 288ebb5a007c 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 07650bc
        Default Java 1.8.0_101
        findbugs v3.0.0
        findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/16638/artifact/patchprocess/new-findbugs-hadoop-hdfs-project_hadoop-hdfs.html
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16638/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16638/console
        Powered by Apache Yetus 0.4.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 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 7m 13s trunk passed +1 compile 0m 44s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 51s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 41s trunk passed +1 javadoc 0m 53s trunk passed +1 mvninstall 0m 46s the patch passed +1 compile 0m 41s the patch passed +1 javac 0m 41s the patch passed +1 checkstyle 0m 22s the patch passed +1 mvnsite 0m 48s the patch passed +1 mvneclipse 0m 9s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 1m 48s hadoop-hdfs-project/hadoop-hdfs generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0) +1 javadoc 0m 51s the patch passed +1 unit 57m 21s hadoop-hdfs in the patch passed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 76m 37s Reason Tests FindBugs module:hadoop-hdfs-project/hadoop-hdfs   org.apache.hadoop.hdfs.server.datanode.DiskBalancer.cancelPlan(String) does not release lock on all paths At DiskBalancer.java:on all paths At DiskBalancer.java: [line 233]   org.apache.hadoop.hdfs.server.datanode.DiskBalancer.shutdown() does not release lock on all paths At DiskBalancer.java:on all paths At DiskBalancer.java: [line 123] Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-9849 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12827120/HDFS-9849.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 288ebb5a007c 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 07650bc Default Java 1.8.0_101 findbugs v3.0.0 findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/16638/artifact/patchprocess/new-findbugs-hadoop-hdfs-project_hadoop-hdfs.html Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16638/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16638/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        yuanbo Yuanbo Liu added a comment -

        upload v2 patch to address issues from findbugs

        Show
        yuanbo Yuanbo Liu added a comment - upload v2 patch to address issues from findbugs
        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 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 8m 50s trunk passed
        +1 compile 0m 53s trunk passed
        +1 checkstyle 0m 30s trunk passed
        +1 mvnsite 1m 4s trunk passed
        +1 mvneclipse 0m 16s trunk passed
        +1 findbugs 2m 2s trunk passed
        +1 javadoc 1m 7s trunk passed
        +1 mvninstall 0m 59s the patch passed
        +1 compile 0m 56s the patch passed
        +1 javac 0m 56s the patch passed
        +1 checkstyle 0m 28s the patch passed
        +1 mvnsite 1m 6s the patch passed
        +1 mvneclipse 0m 12s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 2m 9s the patch passed
        +1 javadoc 1m 5s the patch passed
        -1 unit 79m 8s hadoop-hdfs in the patch failed.
        +1 asflicense 0m 19s The patch does not generate ASF License warnings.
        102m 59s



        Reason Tests
        Failed junit tests hadoop.hdfs.server.diskbalancer.TestDiskBalancerWithMockMover
          hadoop.hdfs.server.diskbalancer.TestDiskBalancerRPC



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Issue HDFS-9849
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12827125/HDFS-9849.002.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux bcc9c2605615 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 / 07650bc
        Default Java 1.8.0_101
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/16639/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16639/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16639/console
        Powered by Apache Yetus 0.4.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 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 8m 50s trunk passed +1 compile 0m 53s trunk passed +1 checkstyle 0m 30s trunk passed +1 mvnsite 1m 4s trunk passed +1 mvneclipse 0m 16s trunk passed +1 findbugs 2m 2s trunk passed +1 javadoc 1m 7s trunk passed +1 mvninstall 0m 59s the patch passed +1 compile 0m 56s the patch passed +1 javac 0m 56s the patch passed +1 checkstyle 0m 28s the patch passed +1 mvnsite 1m 6s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 2m 9s the patch passed +1 javadoc 1m 5s the patch passed -1 unit 79m 8s hadoop-hdfs in the patch failed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 102m 59s Reason Tests Failed junit tests hadoop.hdfs.server.diskbalancer.TestDiskBalancerWithMockMover   hadoop.hdfs.server.diskbalancer.TestDiskBalancerRPC Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-9849 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12827125/HDFS-9849.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux bcc9c2605615 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 / 07650bc Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/16639/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16639/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16639/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        yuanbo Yuanbo Liu added a comment -

        Upload v3 patch for test failures.

        Show
        yuanbo Yuanbo Liu added a comment - Upload v3 patch for test failures.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 10s 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 6m 57s trunk passed
        +1 compile 0m 42s trunk passed
        +1 checkstyle 0m 26s trunk passed
        +1 mvnsite 0m 50s trunk passed
        +1 mvneclipse 0m 12s trunk passed
        +1 findbugs 1m 40s trunk passed
        +1 javadoc 0m 56s trunk passed
        +1 mvninstall 0m 45s the patch passed
        +1 compile 0m 42s the patch passed
        +1 javac 0m 42s the patch passed
        +1 checkstyle 0m 22s the patch passed
        +1 mvnsite 0m 52s the patch passed
        +1 mvneclipse 0m 10s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 50s the patch passed
        +1 javadoc 0m 51s the patch passed
        -1 unit 62m 21s hadoop-hdfs in the patch failed.
        +1 asflicense 0m 18s The patch does not generate ASF License warnings.
        81m 16s



        Reason Tests
        Failed junit tests hadoop.hdfs.TestAppendSnapshotTruncate
          hadoop.hdfs.server.namenode.TestDecommissioningStatus



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Issue HDFS-9849
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12827147/HDFS-9849.003.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 5e2fc64e5a40 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / f955712
        Default Java 1.8.0_101
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/16643/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16643/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16643/console
        Powered by Apache Yetus 0.4.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 10s 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 6m 57s trunk passed +1 compile 0m 42s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 0m 50s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 40s trunk passed +1 javadoc 0m 56s trunk passed +1 mvninstall 0m 45s the patch passed +1 compile 0m 42s the patch passed +1 javac 0m 42s the patch passed +1 checkstyle 0m 22s the patch passed +1 mvnsite 0m 52s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 50s the patch passed +1 javadoc 0m 51s the patch passed -1 unit 62m 21s hadoop-hdfs in the patch failed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 81m 16s Reason Tests Failed junit tests hadoop.hdfs.TestAppendSnapshotTruncate   hadoop.hdfs.server.namenode.TestDecommissioningStatus Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-9849 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12827147/HDFS-9849.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 5e2fc64e5a40 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f955712 Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/16643/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16643/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16643/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        anu Anu Engineer added a comment - - edited

        Yuanbo Liu Sorry for the delay, I will look at this tomorrow and post some feedback. I had a cursory look and I am not sure if we should do this. I will look at this more closely and come up with some suggestions.

        Show
        anu Anu Engineer added a comment - - edited Yuanbo Liu Sorry for the delay, I will look at this tomorrow and post some feedback. I had a cursory look and I am not sure if we should do this. I will look at this more closely and come up with some suggestions.
        Hide
        yuanbo Yuanbo Liu added a comment -

        Anu Engineer Sure, take your time. Thanks for your comment.

        Show
        yuanbo Yuanbo Liu added a comment - Anu Engineer Sure, take your time. Thanks for your comment.
        Hide
        anu Anu Engineer added a comment -

        Yuanbo Liu Thanks for your patience. I am not fully convinced that we should do this. Let us look at the downside of not having this patch. As Arpit Agarwal mentioned it can lock up a requests when we are shutting down datanodes. However if datanode is shutting down, queuing the commands is not really useful right ?. Just to make sure that I am not missing something, what do you think is the real benefit of fixing this issue ?

        Arpit Agarwal I would love to get your perspective on this, the code change is minor, but I am not sure if we are achieving anything significant. An alternative approach would be to do actually not shutdown the thread whenever we are done balancing or in the cancel path – Arpit Agarwal mentioned the same in an earlier code review.

        Show
        anu Anu Engineer added a comment - Yuanbo Liu Thanks for your patience. I am not fully convinced that we should do this. Let us look at the downside of not having this patch. As Arpit Agarwal mentioned it can lock up a requests when we are shutting down datanodes. However if datanode is shutting down, queuing the commands is not really useful right ?. Just to make sure that I am not missing something, what do you think is the real benefit of fixing this issue ? Arpit Agarwal I would love to get your perspective on this, the code change is minor, but I am not sure if we are achieving anything significant. An alternative approach would be to do actually not shutdown the thread whenever we are done balancing or in the cancel path – Arpit Agarwal mentioned the same in an earlier code review.
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        The patch lgtm.

        The risk of not fixing it is very low right now as Anu Engineer mentioned but it's generally good practice not to sleep with a lock held, so I am +1 for fixing it.

        Show
        arpitagarwal Arpit Agarwal added a comment - The patch lgtm. The risk of not fixing it is very low right now as Anu Engineer mentioned but it's generally good practice not to sleep with a lock held, so I am +1 for fixing it.
        Hide
        anu Anu Engineer added a comment -

        Arpit Agarwal Thanks for sharing your view. I agree with the general practice of minimizing the time the lock is held.
        Yuanbo Liu +1, LGTM too, I will commit this patch shortly.

        Show
        anu Anu Engineer added a comment - Arpit Agarwal Thanks for sharing your view. I agree with the general practice of minimizing the time the lock is held. Yuanbo Liu +1, LGTM too, I will commit this patch shortly.
        Hide
        yuanbo Yuanbo Liu added a comment -

        Anu Engineer Yes you're right, since dataXceiverServer is killed before shutting down disk balance, there seems no reason to use lock while stopping balance services. But considering DiskBalancer#shutdown is a public method, we cannot anticipate how it's gonna be used. So it's better to lock up requests in shutdown method. Regarding my patch, I agree with you that it does not have much benefit to fix this issue. Anyway, I'm +0 for my patch

        Show
        yuanbo Yuanbo Liu added a comment - Anu Engineer Yes you're right, since dataXceiverServer is killed before shutting down disk balance, there seems no reason to use lock while stopping balance services. But considering DiskBalancer#shutdown is a public method, we cannot anticipate how it's gonna be used. So it's better to lock up requests in shutdown method. Regarding my patch, I agree with you that it does not have much benefit to fix this issue. Anyway, I'm +0 for my patch
        Hide
        anu Anu Engineer added a comment -

        Yuanbo Liu Thanks for the contribution. I have committed this to trunk.

        Show
        anu Anu Engineer added a comment - Yuanbo Liu Thanks for the contribution. I have committed this to trunk.
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10417 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10417/)
        HDFS-9849. DiskBalancer: reduce lock path in shutdown code. Contributed (aengineer: rev baab48922a301d639ea84ecf00d8a7616acd950d)

        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DiskBalancer.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10417 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10417/ ) HDFS-9849 . DiskBalancer: reduce lock path in shutdown code. Contributed (aengineer: rev baab48922a301d639ea84ecf00d8a7616acd950d) (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DiskBalancer.java

          People

          • Assignee:
            yuanbo Yuanbo Liu
            Reporter:
            anu Anu Engineer
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development