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

Add checkOperation(WRITE) checks in FSNamesystem

Details

    • Reviewed

    Description

      Similar to the work done in HDFS-4591 to avoid having to take a write lock before checking if an operation category is allowed, we can do the same for the write lock that is taken sometimes (when updating access time) within getBlockLocations.

      This is particularly useful when using the standby read feature (HDFS-12943), as it will be the case on an observer node that the operationCategory(READ) check succeeds but the operationCategory(WRITE) check fails. It would be ideal to fail this check before acquiring the write lock.

      Attachments

        1. HDFS-13602.000.patch
          0.7 kB
          Chao Sun
        2. HDFS-13602.001.patch
          4 kB
          Chao Sun

        Issue Links

          Activity

            csun Chao Sun added a comment -

            Thanks xkrogen for creating this JIRA. Submitted patch v0 to do the double-check for getBlockLocations, as suggested in HDFS-4591.

            csun Chao Sun added a comment - Thanks xkrogen for creating this JIRA. Submitted patch v0 to do the double-check for getBlockLocations , as suggested in HDFS-4591 .
            genericqa genericqa added a comment -
            -1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 25s Docker mode activated.
                  Prechecks
            +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.
                  trunk Compile Tests
            +1 mvninstall 27m 3s trunk passed
            +1 compile 0m 56s trunk passed
            +1 checkstyle 0m 55s trunk passed
            +1 mvnsite 1m 5s trunk passed
            +1 shadedclient 12m 22s branch has no errors when building and testing our client artifacts.
            +1 findbugs 2m 6s trunk passed
            +1 javadoc 0m 52s trunk passed
                  Patch Compile Tests
            +1 mvninstall 1m 7s the patch passed
            +1 compile 1m 1s the patch passed
            +1 javac 1m 1s the patch passed
            +1 checkstyle 0m 52s the patch passed
            +1 mvnsite 1m 3s the patch passed
            +1 whitespace 0m 0s The patch has no whitespace issues.
            +1 shadedclient 11m 32s patch has no errors when building and testing our client artifacts.
            +1 findbugs 2m 0s the patch passed
            +1 javadoc 0m 45s the patch passed
                  Other Tests
            -1 unit 99m 39s hadoop-hdfs in the patch failed.
            +1 asflicense 0m 29s The patch does not generate ASF License warnings.
            163m 54s



            Reason Tests
            Failed junit tests hadoop.hdfs.server.namenode.TestNameNodeMetadataConsistency



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd
            JIRA Issue HDFS-13602
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12925025/HDFS-13602.000.patch
            Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
            uname Linux a81f0a7c67f4 3.13.0-143-generic #192-Ubuntu SMP Tue Feb 27 10:45:36 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision trunk / d9852eb
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_162
            findbugs v3.1.0-RC1
            unit https://builds.apache.org/job/PreCommit-HDFS-Build/24295/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/24295/testReport/
            Max. process+thread count 2970 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/24295/console
            Powered by Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org

            This message was automatically generated.

            genericqa genericqa added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 25s Docker mode activated.       Prechecks +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.       trunk Compile Tests +1 mvninstall 27m 3s trunk passed +1 compile 0m 56s trunk passed +1 checkstyle 0m 55s trunk passed +1 mvnsite 1m 5s trunk passed +1 shadedclient 12m 22s branch has no errors when building and testing our client artifacts. +1 findbugs 2m 6s trunk passed +1 javadoc 0m 52s trunk passed       Patch Compile Tests +1 mvninstall 1m 7s the patch passed +1 compile 1m 1s the patch passed +1 javac 1m 1s the patch passed +1 checkstyle 0m 52s the patch passed +1 mvnsite 1m 3s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedclient 11m 32s patch has no errors when building and testing our client artifacts. +1 findbugs 2m 0s the patch passed +1 javadoc 0m 45s the patch passed       Other Tests -1 unit 99m 39s hadoop-hdfs in the patch failed. +1 asflicense 0m 29s The patch does not generate ASF License warnings. 163m 54s Reason Tests Failed junit tests hadoop.hdfs.server.namenode.TestNameNodeMetadataConsistency Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd JIRA Issue HDFS-13602 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12925025/HDFS-13602.000.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle uname Linux a81f0a7c67f4 3.13.0-143-generic #192-Ubuntu SMP Tue Feb 27 10:45:36 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision trunk / d9852eb maven version: Apache Maven 3.3.9 Default Java 1.8.0_162 findbugs v3.1.0-RC1 unit https://builds.apache.org/job/PreCommit-HDFS-Build/24295/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/24295/testReport/ Max. process+thread count 2970 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/24295/console Powered by Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
            xkrogen Erik Krogen added a comment -

            LGTM csun! shv, can you help review?

            xkrogen Erik Krogen added a comment - LGTM csun ! shv , can you help review?

            I see similar problems with

            • concat()
            • createSymlink()
            • updatePipeline()
            • deleteSnapshot()
            • queryRollingUpgrade() - not checking inside the lock
            • add/modify/removeCacheDirective()
            • add/modify/removeCachePool()

            Let's fix those a well.

            shv Konstantin Shvachko added a comment - I see similar problems with concat() createSymlink() updatePipeline() deleteSnapshot() queryRollingUpgrade() - not checking inside the lock add/modify/removeCacheDirective() add/modify/removeCachePool() Let's fix those a well.
            csun Chao Sun added a comment - - edited

            Thanks shv. Uploaded patch v1 to address the comments. I also updated the JIRA title to reflect the newest changes. Let me know if that's OK.

            csun Chao Sun added a comment - - edited Thanks shv . Uploaded patch v1 to address the comments. I also updated the JIRA title to reflect the newest changes. Let me know if that's OK.
            csun Chao Sun added a comment - - edited

            Also, it seems renewLease is checking the wrong operation type:

              /**
               * Renew the lease(s) held by the given client
               */
              void renewLease(String holder) throws IOException {
                checkOperation(OperationCategory.WRITE);
                readLock();
                try {
                  checkOperation(OperationCategory.WRITE);
                  checkNameNodeSafeMode("Cannot renew lease for " + holder);
                  leaseManager.renewLease(holder);
                } finally {
                  readUnlock("renewLease");
                }
              }
            

            Shouldn't it be checkOperation(OperationCategory.READ);?

            csun Chao Sun added a comment - - edited Also, it seems renewLease is checking the wrong operation type: /** * Renew the lease(s) held by the given client */ void renewLease( String holder) throws IOException { checkOperation(OperationCategory.WRITE); readLock(); try { checkOperation(OperationCategory.WRITE); checkNameNodeSafeMode( "Cannot renew lease for " + holder); leaseManager.renewLease(holder); } finally { readUnlock( "renewLease" ); } } Shouldn't it be checkOperation(OperationCategory.READ); ?
            xkrogen Erik Krogen added a comment -

            New scope sounds good to me. csun, renewLease is a modification operation, as it updates the list of leases. It is not necessary to take the FSNameystem write lock since LeaseManager has its own synchronization. See some context in HDFS-5681

            xkrogen Erik Krogen added a comment - New scope sounds good to me. csun , renewLease is a modification operation, as it updates the list of leases. It is not necessary to take the FSNameystem write lock since LeaseManager has its own synchronization. See some context in HDFS-5681
            genericqa genericqa added a comment -
            -1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 20s Docker mode activated.
                  Prechecks
            +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.
                  trunk Compile Tests
            +1 mvninstall 30m 0s trunk passed
            +1 compile 0m 58s trunk passed
            +1 checkstyle 0m 52s trunk passed
            +1 mvnsite 1m 3s trunk passed
            +1 shadedclient 12m 20s branch has no errors when building and testing our client artifacts.
            +1 findbugs 1m 49s trunk passed
            +1 javadoc 0m 45s trunk passed
                  Patch Compile Tests
            +1 mvninstall 0m 58s the patch passed
            +1 compile 0m 50s the patch passed
            +1 javac 0m 50s the patch passed
            +1 checkstyle 0m 47s the patch passed
            +1 mvnsite 0m 58s the patch passed
            +1 whitespace 0m 0s The patch has no whitespace issues.
            +1 shadedclient 11m 12s patch has no errors when building and testing our client artifacts.
            +1 findbugs 2m 17s the patch passed
            +1 javadoc 0m 48s the patch passed
                  Other Tests
            -1 unit 83m 7s hadoop-hdfs in the patch failed.
            +1 asflicense 0m 27s The patch does not generate ASF License warnings.
            149m 14s



            Reason Tests
            Failed junit tests hadoop.hdfs.server.blockmanagement.TestBlockTokenWithDFSStriped
              hadoop.hdfs.server.datanode.TestDirectoryScanner



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd
            JIRA Issue HDFS-13602
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12925633/HDFS-13602.001.patch
            Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
            uname Linux 3ba46901d3df 3.13.0-139-generic #188-Ubuntu SMP Tue Jan 9 14:43:09 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision trunk / 135941e
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_162
            findbugs v3.1.0-RC1
            unit https://builds.apache.org/job/PreCommit-HDFS-Build/24328/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/24328/testReport/
            Max. process+thread count 3732 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/24328/console
            Powered by Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org

            This message was automatically generated.

            genericqa genericqa added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated.       Prechecks +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.       trunk Compile Tests +1 mvninstall 30m 0s trunk passed +1 compile 0m 58s trunk passed +1 checkstyle 0m 52s trunk passed +1 mvnsite 1m 3s trunk passed +1 shadedclient 12m 20s branch has no errors when building and testing our client artifacts. +1 findbugs 1m 49s trunk passed +1 javadoc 0m 45s trunk passed       Patch Compile Tests +1 mvninstall 0m 58s the patch passed +1 compile 0m 50s the patch passed +1 javac 0m 50s the patch passed +1 checkstyle 0m 47s the patch passed +1 mvnsite 0m 58s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedclient 11m 12s patch has no errors when building and testing our client artifacts. +1 findbugs 2m 17s the patch passed +1 javadoc 0m 48s the patch passed       Other Tests -1 unit 83m 7s hadoop-hdfs in the patch failed. +1 asflicense 0m 27s The patch does not generate ASF License warnings. 149m 14s Reason Tests Failed junit tests hadoop.hdfs.server.blockmanagement.TestBlockTokenWithDFSStriped   hadoop.hdfs.server.datanode.TestDirectoryScanner Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd JIRA Issue HDFS-13602 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12925633/HDFS-13602.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle uname Linux 3ba46901d3df 3.13.0-139-generic #188-Ubuntu SMP Tue Jan 9 14:43:09 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision trunk / 135941e maven version: Apache Maven 3.3.9 Default Java 1.8.0_162 findbugs v3.1.0-RC1 unit https://builds.apache.org/job/PreCommit-HDFS-Build/24328/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/24328/testReport/ Max. process+thread count 3732 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/24328/console Powered by Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
            csun Chao Sun added a comment -

            Thanks xkrogen for the explanation. I should have checked HDFS-5681 before the comment.

            csun Chao Sun added a comment - Thanks xkrogen for the explanation. I should have checked HDFS-5681 before the comment.

            +1 will commit in a bit.

            shv Konstantin Shvachko added a comment - +1 will commit in a bit.

            I just committed this. Looks like we had this in all versions. Backported down to branch 2.7.
            Thank you csun.

            shv Konstantin Shvachko added a comment - I just committed this. Looks like we had this in all versions. Backported down to branch 2.7. Thank you csun .
            hudson Hudson added a comment -

            SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #14332 (See https://builds.apache.org/job/Hadoop-trunk-Commit/14332/)
            HDFS-13602. Add checkOperation(WRITE) checks in FSNamesystem. (shv: rev ff013d2c952272f3176dcf624251b05d610503b5)

            • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
            hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #14332 (See https://builds.apache.org/job/Hadoop-trunk-Commit/14332/ ) HDFS-13602 . Add checkOperation(WRITE) checks in FSNamesystem. (shv: rev ff013d2c952272f3176dcf624251b05d610503b5) (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java

            People

              csun Chao Sun
              xkrogen Erik Krogen
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: