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

DatanodeStorage.isValidStorageId() is broken

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: hdfs-client
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      After HDFS-8979, the check is returning true for the old storage ID format. So storage IDs in the old format won't be updated during datanode upgrade.

      1. HDFS-9739.000.patch
        0.9 kB
        Mingliang Liu

        Issue Links

          Activity

          Hide
          kihwal Kihwal Lee added a comment - - edited

          Mingliang Liu Can you take a look?

          Show
          kihwal Kihwal Lee added a comment - - edited Mingliang Liu Can you take a look?
          Hide
          liuml07 Mingliang Liu added a comment -

          Thanks Kihwal Lee for reporting this.

          The patch HDFS-8979 simply removes one line of code: UUID.fromString(storageID.substring(STORAGE_ID_PREFIX.length())); This was removed because I thought it's not needed as we create a UUID and then ignore it.

          Actually, if the storageID stripped prefix does not conform to the UUID string representation (old format?), the DatanodeStorage#isValidStorageId() should return to false. The old code will handle this thanks to thrown IllegalArgumentException, while the patch HDFS-8979 removes this check.

          A simple fix is to added the {{UUID.fromString() back.

          Show
          liuml07 Mingliang Liu added a comment - Thanks Kihwal Lee for reporting this. The patch HDFS-8979 simply removes one line of code: UUID.fromString(storageID.substring(STORAGE_ID_PREFIX.length())); This was removed because I thought it's not needed as we create a UUID and then ignore it. Actually, if the storageID stripped prefix does not conform to the UUID string representation (old format?), the DatanodeStorage#isValidStorageId() should return to false. The old code will handle this thanks to thrown IllegalArgumentException , while the patch HDFS-8979 removes this check. A simple fix is to added the {{ UUID.fromString() back.
          Hide
          liuml07 Mingliang Liu added a comment -

          The v0 patch simply adds the UUID.fromString back.

          I think we any unit tests that can cover this will be very helpful. Any suggestion Kihwal Lee?

          Show
          liuml07 Mingliang Liu added a comment - The v0 patch simply adds the UUID.fromString back. I think we any unit tests that can cover this will be very helpful. Any suggestion Kihwal Lee ?
          Hide
          kihwal Kihwal Lee added a comment -

          If the patch works, TestDatanodeStartupFixesLegacyStorageIDs should fail until HDFS-9730 is fixed. This wasn't failing in trunk and brach-2 despite the bug because of the broken isValidStorageId().

          Show
          kihwal Kihwal Lee added a comment - If the patch works, TestDatanodeStartupFixesLegacyStorageIDs should fail until HDFS-9730 is fixed. This wasn't failing in trunk and brach-2 despite the bug because of the broken isValidStorageId() .
          Hide
          liuml07 Mingliang Liu added a comment -

          Thanks for your prompt reply, Kihwal Lee. I tested locally and TestDatanodeStartupFixesLegacyStorageIDs failed as expected. Glad to know HDFS-9730 will address this further.

          Show
          liuml07 Mingliang Liu added a comment - Thanks for your prompt reply, Kihwal Lee . I tested locally and TestDatanodeStartupFixesLegacyStorageIDs failed as expected. Glad to know HDFS-9730 will address this further.
          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 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.
          0 mvndep 0m 13s Maven dependency ordering for branch
          +1 mvninstall 9m 52s trunk passed
          +1 compile 0m 49s trunk passed with JDK v1.8.0_66
          +1 compile 0m 42s trunk passed with JDK v1.7.0_91
          +1 checkstyle 0m 19s trunk passed
          +1 mvnsite 0m 48s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 2m 33s trunk passed
          +1 javadoc 0m 35s trunk passed with JDK v1.8.0_66
          +1 javadoc 0m 36s trunk passed with JDK v1.7.0_91
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 0m 44s the patch passed
          +1 compile 0m 47s the patch passed with JDK v1.8.0_66
          +1 javac 0m 47s the patch passed
          +1 compile 0m 37s the patch passed with JDK v1.7.0_91
          +1 javac 0m 37s the patch passed
          +1 checkstyle 0m 17s the patch passed
          +1 mvnsite 0m 45s the patch passed
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s Patch has no whitespace issues.
          +1 findbugs 2m 51s the patch passed
          +1 javadoc 0m 33s the patch passed with JDK v1.8.0_66
          +1 javadoc 0m 32s the patch passed with JDK v1.7.0_91
          +1 unit 1m 19s hadoop-hdfs-client in the patch passed with JDK v1.8.0_66.
          +1 unit 1m 12s hadoop-hdfs-client in the patch passed with JDK v1.7.0_91.
          +1 asflicense 0m 23s Patch does not generate ASF License warnings.
          30m 21s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:0ca8df7
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12785848/HDFS-9739.000.patch
          JIRA Issue HDFS-9739
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux e2016d74751a 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 / 4ae543f
          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
          JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/14349/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs-client U: hadoop-hdfs-project/hadoop-hdfs-client
          Max memory used 76MB
          Powered by Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/14349/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 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. 0 mvndep 0m 13s Maven dependency ordering for branch +1 mvninstall 9m 52s trunk passed +1 compile 0m 49s trunk passed with JDK v1.8.0_66 +1 compile 0m 42s trunk passed with JDK v1.7.0_91 +1 checkstyle 0m 19s trunk passed +1 mvnsite 0m 48s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 2m 33s trunk passed +1 javadoc 0m 35s trunk passed with JDK v1.8.0_66 +1 javadoc 0m 36s trunk passed with JDK v1.7.0_91 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 0m 44s the patch passed +1 compile 0m 47s the patch passed with JDK v1.8.0_66 +1 javac 0m 47s the patch passed +1 compile 0m 37s the patch passed with JDK v1.7.0_91 +1 javac 0m 37s the patch passed +1 checkstyle 0m 17s the patch passed +1 mvnsite 0m 45s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 2m 51s the patch passed +1 javadoc 0m 33s the patch passed with JDK v1.8.0_66 +1 javadoc 0m 32s the patch passed with JDK v1.7.0_91 +1 unit 1m 19s hadoop-hdfs-client in the patch passed with JDK v1.8.0_66. +1 unit 1m 12s hadoop-hdfs-client in the patch passed with JDK v1.7.0_91. +1 asflicense 0m 23s Patch does not generate ASF License warnings. 30m 21s Subsystem Report/Notes Docker Image:yetus/hadoop:0ca8df7 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12785848/HDFS-9739.000.patch JIRA Issue HDFS-9739 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux e2016d74751a 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 / 4ae543f 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 JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/14349/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs-client U: hadoop-hdfs-project/hadoop-hdfs-client Max memory used 76MB Powered by Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org Console output https://builds.apache.org/job/PreCommit-HDFS-Build/14349/console This message was automatically generated.
          Hide
          liuml07 Mingliang Liu added a comment -

          TestDatanodeStartupFixesLegacyStorageIDs is in hadoop-hdfs module and this patch did not trigger its run as the change is in hadoop-hdfs-client module.

          Show
          liuml07 Mingliang Liu added a comment - TestDatanodeStartupFixesLegacyStorageIDs is in hadoop-hdfs module and this patch did not trigger its run as the change is in hadoop-hdfs-client module.
          Hide
          vinayrpet Vinayakumar B added a comment -

          +1, for the patch.
          I have also verified locally. TestDatanodeStartupFixesLegacyStorageIDs fails with this patch as expected. And further passes with HDFS-9730 patch.

          Going to commit it soon.

          Show
          vinayrpet Vinayakumar B added a comment - +1, for the patch. I have also verified locally. TestDatanodeStartupFixesLegacyStorageIDs fails with this patch as expected. And further passes with HDFS-9730 patch. Going to commit it soon.
          Hide
          vinayrpet Vinayakumar B added a comment -

          Committed to trunk, branch-2 and branch-2.8

          Thanks Kihwal Lee for reporting and review.
          Thanks Mingliang Liu for the fix.

          Show
          vinayrpet Vinayakumar B added a comment - Committed to trunk, branch-2 and branch-2.8 Thanks Kihwal Lee for reporting and review. Thanks Mingliang Liu for the fix.
          Hide
          liuml07 Mingliang Liu added a comment -

          Thanks for the verification, review and commit, Vinayakumar B!

          Show
          liuml07 Mingliang Liu added a comment - Thanks for the verification, review and commit, Vinayakumar B !
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #9235 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9235/)
          HDFS-9739. DatanodeStorage.isValidStorageId() is broken (Contributed by (vinayakumarb: rev d6b1acb940180befeb4c855d0e4a339dbc035e7d)

          • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/server/protocol/DatanodeStorage.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #9235 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9235/ ) HDFS-9739 . DatanodeStorage.isValidStorageId() is broken (Contributed by (vinayakumarb: rev d6b1acb940180befeb4c855d0e4a339dbc035e7d) hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/server/protocol/DatanodeStorage.java

            People

            • Assignee:
              liuml07 Mingliang Liu
              Reporter:
              kihwal Kihwal Lee
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development