Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-11047

Remove deep copies of FinalizedReplica to alleviate heap consumption on DataNode

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.8.0, 3.0.0-alpha2
    • Component/s: datanode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      DirectoryScanner does scan by deep copying FinalizedReplica. In a deployment with 500,000+ blocks, we've seen the DN heap usage being accumulated to high peaks very quickly. Deep copies of FinalizedReplica will make DN heap usage even worse if directory scans are scheduled more frequently. This proposes removing unnecessary deep copies since DirectoryScanner#scan already holds lock of dataset.

      DirectoryScanner#scan

          try(AutoCloseableLock lock = dataset.acquireDatasetLock()) {
            for (Entry<String, ScanInfo[]> entry : diskReport.entrySet()) {
              String bpid = entry.getKey();
              ScanInfo[] blockpoolReport = entry.getValue();
              
              Stats statsRecord = new Stats(bpid);
              stats.put(bpid, statsRecord);
              LinkedList<ScanInfo> diffRecord = new LinkedList<ScanInfo>();
              diffs.put(bpid, diffRecord);
              
              statsRecord.totalBlocks = blockpoolReport.length;
              List<ReplicaInfo> bl = dataset.getFinalizedBlocks(bpid); /* deep copies here*/
      

      FsDatasetImpl#getFinalizedBlocks

        public List<ReplicaInfo> getFinalizedBlocks(String bpid) {
          try (AutoCloseableLock lock = datasetLock.acquire()) {
            ArrayList<ReplicaInfo> finalized =
                new ArrayList<ReplicaInfo>(volumeMap.size(bpid));
            for (ReplicaInfo b : volumeMap.replicas(bpid)) {
              if (b.getState() == ReplicaState.FINALIZED) {
                finalized.add(new ReplicaBuilder(ReplicaState.FINALIZED)
                    .from(b).build()); /* deep copies here*/
              }
            }
            return finalized;
          }
        }
      
      1. HDFS-11047.000.patch
        7 kB
        Xiaobing Zhou
      2. HDFS-11047.001.patch
        5 kB
        Xiaobing Zhou
      3. HDFS-11047.002.patch
        5 kB
        Xiaobing Zhou
      4. HDFS-11047-branch-2.002.patch
        5 kB
        Xiaobing Zhou

        Activity

        Hide
        brahmareddy Brahma Reddy Battula added a comment -

        IMHO, this will be good candidate to go branch-2.7. Xiaobing Zhou can you update the branch-2.7 patch also..?

        Show
        brahmareddy Brahma Reddy Battula added a comment - IMHO, this will be good candidate to go branch-2.7. Xiaobing Zhou can you update the branch-2.7 patch also..?
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 14m 42s 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 49s branch-2 passed
        +1 compile 0m 50s branch-2 passed with JDK v1.8.0_111
        +1 compile 0m 50s branch-2 passed with JDK v1.7.0_111
        +1 checkstyle 0m 33s branch-2 passed
        +1 mvnsite 0m 57s branch-2 passed
        +1 mvneclipse 0m 17s branch-2 passed
        +1 findbugs 2m 9s branch-2 passed
        +1 javadoc 1m 3s branch-2 passed with JDK v1.8.0_111
        +1 javadoc 1m 55s branch-2 passed with JDK v1.7.0_111
        +1 mvninstall 0m 52s the patch passed
        +1 compile 0m 43s the patch passed with JDK v1.8.0_111
        +1 javac 0m 43s the patch passed
        +1 compile 0m 48s the patch passed with JDK v1.7.0_111
        +1 javac 0m 48s the patch passed
        +1 checkstyle 0m 30s hadoop-hdfs-project/hadoop-hdfs: The patch generated 0 new + 180 unchanged - 1 fixed = 180 total (was 181)
        +1 mvnsite 0m 58s the patch passed
        +1 mvneclipse 0m 15s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 2m 30s the patch passed
        +1 javadoc 0m 59s the patch passed with JDK v1.8.0_111
        +1 javadoc 1m 49s the patch passed with JDK v1.7.0_111
        +1 unit 50m 3s hadoop-hdfs in the patch passed with JDK v1.7.0_111.
        +1 asflicense 0m 21s The patch does not generate ASF License warnings.
        145m 55s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:b59b8b7
        JIRA Issue HDFS-11047
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835692/HDFS-11047-branch-2.002.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux dcf06a0b364b 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 branch-2 / ce8ace3
        Default Java 1.7.0_111
        Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_111 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_111
        findbugs v3.0.0
        JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17334/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17334/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 14m 42s 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 49s branch-2 passed +1 compile 0m 50s branch-2 passed with JDK v1.8.0_111 +1 compile 0m 50s branch-2 passed with JDK v1.7.0_111 +1 checkstyle 0m 33s branch-2 passed +1 mvnsite 0m 57s branch-2 passed +1 mvneclipse 0m 17s branch-2 passed +1 findbugs 2m 9s branch-2 passed +1 javadoc 1m 3s branch-2 passed with JDK v1.8.0_111 +1 javadoc 1m 55s branch-2 passed with JDK v1.7.0_111 +1 mvninstall 0m 52s the patch passed +1 compile 0m 43s the patch passed with JDK v1.8.0_111 +1 javac 0m 43s the patch passed +1 compile 0m 48s the patch passed with JDK v1.7.0_111 +1 javac 0m 48s the patch passed +1 checkstyle 0m 30s hadoop-hdfs-project/hadoop-hdfs: The patch generated 0 new + 180 unchanged - 1 fixed = 180 total (was 181) +1 mvnsite 0m 58s the patch passed +1 mvneclipse 0m 15s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 2m 30s the patch passed +1 javadoc 0m 59s the patch passed with JDK v1.8.0_111 +1 javadoc 1m 49s the patch passed with JDK v1.7.0_111 +1 unit 50m 3s hadoop-hdfs in the patch passed with JDK v1.7.0_111. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 145m 55s Subsystem Report/Notes Docker Image:yetus/hadoop:b59b8b7 JIRA Issue HDFS-11047 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835692/HDFS-11047-branch-2.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux dcf06a0b364b 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 branch-2 / ce8ace3 Default Java 1.7.0_111 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_111 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_111 findbugs v3.0.0 JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17334/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17334/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        liuml07 Mingliang Liu added a comment -

        +1 pending on Jenkins.

        Show
        liuml07 Mingliang Liu added a comment - +1 pending on Jenkins.
        Hide
        xiaobingo Xiaobing Zhou added a comment -

        Thanks Mingliang Liu for committing it, Joe Pallas and Arpit Agarwal for reviewing.
        I posted branch-2 patch.

        Show
        xiaobingo Xiaobing Zhou added a comment - Thanks Mingliang Liu for committing it, Joe Pallas and Arpit Agarwal for reviewing. I posted branch-2 patch.
        Hide
        liuml07 Mingliang Liu added a comment -

        Committed to trunk branch. Thanks Xiaobing Zhou for contribution. Thanks Joe Pallas and Arpit Agarwal for the review.

        Can you provide a branch-2 patch as I see non-trivial conflicts when cherry-picking. Will leave this JIRA open before that.

        Show
        liuml07 Mingliang Liu added a comment - Committed to trunk branch. Thanks Xiaobing Zhou for contribution. Thanks Joe Pallas and Arpit Agarwal for the review. Can you provide a branch-2 patch as I see non-trivial conflicts when cherry-picking. Will leave this JIRA open before that.
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10712 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10712/)
        HDFS-11047. Remove deep copies of FinalizedReplica to alleviate heap (liuml07: rev 9e03ee527988ff85af7f2c224c5570b69d09279a)

        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java
        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DirectoryScanner.java
        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/FsDatasetSpi.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10712 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10712/ ) HDFS-11047 . Remove deep copies of FinalizedReplica to alleviate heap (liuml07: rev 9e03ee527988ff85af7f2c224c5570b69d09279a) (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DirectoryScanner.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/FsDatasetSpi.java
        Hide
        liuml07 Mingliang Liu added a comment -

        +1 Will commit in a sec.

        Show
        liuml07 Mingliang Liu added a comment - +1 Will commit in a sec.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 11s 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 17s trunk passed
        +1 compile 0m 47s trunk passed
        +1 checkstyle 0m 29s trunk passed
        +1 mvnsite 0m 57s trunk passed
        +1 mvneclipse 0m 14s trunk passed
        +1 findbugs 1m 49s trunk passed
        +1 javadoc 0m 40s trunk passed
        +1 mvninstall 0m 46s the patch passed
        +1 compile 0m 43s the patch passed
        +1 javac 0m 43s the patch passed
        +1 checkstyle 0m 25s the patch passed
        +1 mvnsite 0m 53s the patch passed
        +1 mvneclipse 0m 12s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 54s the patch passed
        +1 javadoc 0m 35s the patch passed
        +1 unit 64m 12s hadoop-hdfs in the patch passed.
        +1 asflicense 0m 17s The patch does not generate ASF License warnings.
        83m 30s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Issue HDFS-11047
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835652/HDFS-11047.002.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 0f8588aa03e2 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 / dd4ed6a
        Default Java 1.8.0_101
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17333/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17333/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 11s 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 17s trunk passed +1 compile 0m 47s trunk passed +1 checkstyle 0m 29s trunk passed +1 mvnsite 0m 57s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 49s trunk passed +1 javadoc 0m 40s trunk passed +1 mvninstall 0m 46s the patch passed +1 compile 0m 43s the patch passed +1 javac 0m 43s the patch passed +1 checkstyle 0m 25s the patch passed +1 mvnsite 0m 53s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 54s the patch passed +1 javadoc 0m 35s the patch passed +1 unit 64m 12s hadoop-hdfs in the patch passed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 83m 30s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-11047 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835652/HDFS-11047.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 0f8588aa03e2 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 / dd4ed6a Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17333/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17333/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        +1 for the v002 patch pending Jenkins. Thanks Xiaobing Zhou.

        Show
        arpitagarwal Arpit Agarwal added a comment - +1 for the v002 patch pending Jenkins. Thanks Xiaobing Zhou .
        Hide
        xiaobingo Xiaobing Zhou added a comment -

        v002 patch addressed that, thanks Arpit Agarwal

        Show
        xiaobingo Xiaobing Zhou added a comment - v002 patch addressed that, thanks Arpit Agarwal
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        The v2 patch looks good.

        Nitpick: can you please change the may need to to should?

          /**
           * Gets a list of references to the finalized blocks for the given block pool.
           * <p>
           * Callers of this function should call
           * {@link FsDatasetSpi#acquireDatasetLock} to avoid blocks' status being
           * changed during list iteration.
        

        Also can you please copy the documentation to FsDatasetImpl#getFinalizedBlocks so future callers are less likely to miss the caveat?

        Show
        arpitagarwal Arpit Agarwal added a comment - The v2 patch looks good. Nitpick: can you please change the may need to to should ? /** * Gets a list of references to the finalized blocks for the given block pool. * <p> * Callers of this function should call * {@link FsDatasetSpi#acquireDatasetLock} to avoid blocks' status being * changed during list iteration. Also can you please copy the documentation to FsDatasetImpl#getFinalizedBlocks so future callers are less likely to miss the caveat?
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 14s 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 3s trunk passed
        +1 compile 0m 47s trunk passed
        +1 checkstyle 0m 27s trunk passed
        +1 mvnsite 0m 53s trunk passed
        +1 mvneclipse 0m 13s trunk passed
        +1 findbugs 1m 45s trunk passed
        +1 javadoc 0m 47s trunk passed
        +1 mvninstall 1m 1s the patch passed
        +1 compile 0m 58s the patch passed
        +1 javac 0m 58s the patch passed
        +1 checkstyle 0m 30s the patch passed
        +1 mvnsite 1m 7s the patch passed
        +1 mvneclipse 0m 11s the patch passed
        +1 whitespace 0m 1s The patch has no whitespace issues.
        +1 findbugs 1m 56s the patch passed
        +1 javadoc 0m 38s the patch passed
        -1 unit 63m 27s hadoop-hdfs in the patch failed.
        +1 asflicense 0m 19s The patch does not generate ASF License warnings.
        83m 37s



        Reason Tests
        Failed junit tests hadoop.hdfs.TestFileCreationDelete
          hadoop.security.TestPermission



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Issue HDFS-11047
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835618/HDFS-11047.001.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 707a5c1e3764 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 / ac35ee9
        Default Java 1.8.0_101
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/17328/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17328/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17328/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 14s 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 3s trunk passed +1 compile 0m 47s trunk passed +1 checkstyle 0m 27s trunk passed +1 mvnsite 0m 53s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 45s trunk passed +1 javadoc 0m 47s trunk passed +1 mvninstall 1m 1s the patch passed +1 compile 0m 58s the patch passed +1 javac 0m 58s the patch passed +1 checkstyle 0m 30s the patch passed +1 mvnsite 1m 7s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 1s The patch has no whitespace issues. +1 findbugs 1m 56s the patch passed +1 javadoc 0m 38s the patch passed -1 unit 63m 27s hadoop-hdfs in the patch failed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 83m 37s Reason Tests Failed junit tests hadoop.hdfs.TestFileCreationDelete   hadoop.security.TestPermission Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-11047 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835618/HDFS-11047.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 707a5c1e3764 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 / ac35ee9 Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/17328/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17328/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17328/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        xiaobingo Xiaobing Zhou added a comment -

        I posted patch v001.

        1. removed deep copies from getFinalizedBlocks.
        2. added contract doc to it.
        3. removed getFinalizedBlocksReferences added in previous patch v000.

        Thanks Arpit Agarwal/Joe Pallas for the reviews.

        Show
        xiaobingo Xiaobing Zhou added a comment - I posted patch v001. removed deep copies from getFinalizedBlocks. added contract doc to it. removed getFinalizedBlocksReferences added in previous patch v000. Thanks Arpit Agarwal / Joe Pallas for the reviews.
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        Nice catch Xiaobing Zhou. Thanks for reporting and fixing this.

        I agree with Joe Pallas that we can just change the behavior of getFinalizedBlocks as it is a private interface. We can document the requirement that the caller of getFinalizedBlocks first get the dataset lock via FsDatasetSpi#acquireDatasetLock.

        In addition to the deep copy there is an apparently unnecessary list to array conversion that you removed. I wasn't able to follow the source history past 2011 to see why it was introduced. IAC I can't think of any reason to retain it.

        Show
        arpitagarwal Arpit Agarwal added a comment - Nice catch Xiaobing Zhou . Thanks for reporting and fixing this. I agree with Joe Pallas that we can just change the behavior of getFinalizedBlocks as it is a private interface. We can document the requirement that the caller of getFinalizedBlocks first get the dataset lock via FsDatasetSpi#acquireDatasetLock . In addition to the deep copy there is an apparently unnecessary list to array conversion that you removed. I wasn't able to follow the source history past 2011 to see why it was introduced. IAC I can't think of any reason to retain it.
        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 2 new or modified test files.
        +1 mvninstall 7m 9s trunk passed
        +1 compile 0m 43s trunk passed
        +1 checkstyle 0m 28s trunk passed
        +1 mvnsite 0m 51s trunk passed
        +1 mvneclipse 0m 13s trunk passed
        +1 findbugs 1m 46s trunk passed
        +1 javadoc 0m 41s trunk passed
        +1 mvninstall 0m 56s the patch passed
        +1 compile 0m 48s the patch passed
        +1 javac 0m 48s the patch passed
        +1 checkstyle 0m 28s the patch passed
        +1 mvnsite 0m 55s the patch passed
        +1 mvneclipse 0m 11s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 58s the patch passed
        +1 javadoc 0m 40s the patch passed
        -1 unit 52m 15s hadoop-hdfs in the patch failed.
        +1 asflicense 0m 21s The patch does not generate ASF License warnings.
        71m 58s



        Reason Tests
        Failed junit tests hadoop.hdfs.server.datanode.TestFsDatasetCache
        Timed out junit tests org.apache.hadoop.hdfs.server.blockmanagement.TestReplicationPolicy
          org.apache.hadoop.tools.TestHdfsConfigFields
          org.apache.hadoop.hdfs.server.blockmanagement.TestBlockTokenWithDFSStriped
          org.apache.hadoop.hdfs.server.blockmanagement.TestOverReplicatedBlocks
          org.apache.hadoop.hdfs.server.blockmanagement.TestBlockStatsMXBean
          org.apache.hadoop.hdfs.server.blockmanagement.TestReplicationPolicyWithNodeGroup



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Issue HDFS-11047
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835215/HDFS-11047.000.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux b78a03837e2d 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 / 084bdab
        Default Java 1.8.0_101
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/17285/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17285/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17285/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 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 2 new or modified test files. +1 mvninstall 7m 9s trunk passed +1 compile 0m 43s trunk passed +1 checkstyle 0m 28s trunk passed +1 mvnsite 0m 51s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 46s trunk passed +1 javadoc 0m 41s trunk passed +1 mvninstall 0m 56s the patch passed +1 compile 0m 48s the patch passed +1 javac 0m 48s the patch passed +1 checkstyle 0m 28s the patch passed +1 mvnsite 0m 55s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 58s the patch passed +1 javadoc 0m 40s the patch passed -1 unit 52m 15s hadoop-hdfs in the patch failed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 71m 58s Reason Tests Failed junit tests hadoop.hdfs.server.datanode.TestFsDatasetCache Timed out junit tests org.apache.hadoop.hdfs.server.blockmanagement.TestReplicationPolicy   org.apache.hadoop.tools.TestHdfsConfigFields   org.apache.hadoop.hdfs.server.blockmanagement.TestBlockTokenWithDFSStriped   org.apache.hadoop.hdfs.server.blockmanagement.TestOverReplicatedBlocks   org.apache.hadoop.hdfs.server.blockmanagement.TestBlockStatsMXBean   org.apache.hadoop.hdfs.server.blockmanagement.TestReplicationPolicyWithNodeGroup Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-11047 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835215/HDFS-11047.000.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux b78a03837e2d 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 / 084bdab Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/17285/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17285/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17285/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        xiaobingo Xiaobing Zhou added a comment -

        Thanks Joe Pallas. I also did that check and tried to modify getFinalizedBlocks directly, however, to be safe, let's poll more inputs to the final choice.

        Show
        xiaobingo Xiaobing Zhou added a comment - Thanks Joe Pallas . I also did that check and tried to modify getFinalizedBlocks directly, however, to be safe, let's poll more inputs to the final choice.
        Hide
        jpallas Joe Pallas added a comment -

        It looks like the directory scanner and one test are the only callers of getFinalizedBlocks. If that's the case, there's no real reason to keep an old version for compatibility. What do you think, Xiaobing Zhou?

        Show
        jpallas Joe Pallas added a comment - It looks like the directory scanner and one test are the only callers of getFinalizedBlocks. If that's the case, there's no real reason to keep an old version for compatibility. What do you think, Xiaobing Zhou ?
        Hide
        xiaobingo Xiaobing Zhou added a comment -

        I posted patch v000, please kindly review it, thanks.
        1. It added new function (i.e. getFinalizedBlocksReferences) to declare references only to replicas instead of changing existing getFinalizedBlocks to avoid any compatibility issues.
        2. Meanwhile, comments are added to getFinalizedBlocks regarding the deep copies contract it already implemented.
        3. removed List -> Array translation.

        Mingliang Liu thank you for the comments.

        Show
        xiaobingo Xiaobing Zhou added a comment - I posted patch v000, please kindly review it, thanks. 1. It added new function (i.e. getFinalizedBlocksReferences) to declare references only to replicas instead of changing existing getFinalizedBlocks to avoid any compatibility issues. 2. Meanwhile, comments are added to getFinalizedBlocks regarding the deep copies contract it already implemented. 3. removed List -> Array translation. Mingliang Liu thank you for the comments.
        Hide
        liuml07 Mingliang Liu added a comment -

        +1 for the proposal. Nice catch. Wondering if getFinalizedBlocks has promised anything regarding the returned value. Another point is about the List -> Array in DirectoryScanner#scan; can we avoid this?

        Show
        liuml07 Mingliang Liu added a comment - +1 for the proposal. Nice catch. Wondering if getFinalizedBlocks has promised anything regarding the returned value. Another point is about the List -> Array in DirectoryScanner#scan ; can we avoid this?

          People

          • Assignee:
            xiaobingo Xiaobing Zhou
            Reporter:
            xiaobingo Xiaobing Zhou
          • Votes:
            0 Vote for this issue
            Watchers:
            11 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development