Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-3202

NamespaceInfo PB translation drops build version

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-alpha
    • Fix Version/s: 2.0.0-alpha
    • Component/s: datanode
    • Labels:
      None

      Description

      The PBHelper#convert(NamespaceInfoProto) function doesn't pass the build version from the NamespaceInfoProto to the created NamespaceInfo object. Instead, the NamespaceInfo constructor gets the build version using the static function Storage#getBuildVersion. DNs also use this static function to determine their own build version. This means that the check the DN does to compare its own build version to that of the NN always passes, regardless of what build version exists on the NN.

      1. HDFS-3202.patch
        2 kB
        Aaron T. Myers

        Issue Links

          Activity

          Aaron T. Myers created issue -
          Aaron T. Myers made changes -
          Field Original Value New Value
          Assignee Aaron T. Myers [ atm ]
          Hide
          Aaron T. Myers added a comment -

          Here's a patch which addresses the issue.

          I'm struggling with how to write a test case for this, since both the NN and DN use a static function to determine their own build version. Posting a patch now while I think about how to do so. Suggestions are certainly welcome.

          I tested this by adding some debugging output to BPServiceActor#checkNNVersion that outputs the NN's and DN's build versions. I confirmed that without the patch the two versions are always the same, and with the patch the NN version is correctly different from the DN version if the NN's build version is different.

          Show
          Aaron T. Myers added a comment - Here's a patch which addresses the issue. I'm struggling with how to write a test case for this, since both the NN and DN use a static function to determine their own build version. Posting a patch now while I think about how to do so. Suggestions are certainly welcome. I tested this by adding some debugging output to BPServiceActor#checkNNVersion that outputs the NN's and DN's build versions. I confirmed that without the patch the two versions are always the same, and with the patch the NN version is correctly different from the DN version if the NN's build version is different.
          Aaron T. Myers made changes -
          Attachment HDFS-3202.patch [ 12521443 ]
          Aaron T. Myers made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Aaron T. Myers made changes -
          Link This issue blocks HDFS-2983 [ HDFS-2983 ]
          Hide
          Suresh Srinivas added a comment -

          +1 for the patch. I think this is trivial enough and unit test is an overkill. One quick comment - do we need second variant of the NamespaceInfo constructor? If it is not used at many places, can we just have one constructor?

          Show
          Suresh Srinivas added a comment - +1 for the patch. I think this is trivial enough and unit test is an overkill. One quick comment - do we need second variant of the NamespaceInfo constructor? If it is not used at many places, can we just have one constructor?
          Hide
          Aaron T. Myers added a comment -

          Thanks a lot for the quick review, Suresh.

          I think this is trivial enough and unit test is an overkill.

          OK, works for me. Writing a proper test here would probably require somehow recompiling and forking a new JVM, etc. which I agree doesn't seem worth it.

          One quick comment - do we need second variant of the NamespaceInfo constructor? If it is not used at many places, can we just have one constructor?

          This variant is currently used in 7 places - 4 in tests and 3 in functional areas of the code. I'm inclined to leave it as-is. Is that OK by you, Suresh?

          Show
          Aaron T. Myers added a comment - Thanks a lot for the quick review, Suresh. I think this is trivial enough and unit test is an overkill. OK, works for me. Writing a proper test here would probably require somehow recompiling and forking a new JVM, etc. which I agree doesn't seem worth it. One quick comment - do we need second variant of the NamespaceInfo constructor? If it is not used at many places, can we just have one constructor? This variant is currently used in 7 places - 4 in tests and 3 in functional areas of the code. I'm inclined to leave it as-is. Is that OK by you, Suresh?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12521443/HDFS-3202.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. 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 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in .

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2191//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2191//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12521443/HDFS-3202.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. 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 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2191//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2191//console This message is automatically generated.
          Hide
          Aaron T. Myers added a comment -

          I've just committed this to trunk and branch-2. Thanks a lot for the review, Suresh.

          Suresh, if you feel strongly about removing one of the NamespaceInfo constructor variants, I'd be happy to do that in a follow-up JIRA.

          Show
          Aaron T. Myers added a comment - I've just committed this to trunk and branch-2. Thanks a lot for the review, Suresh. Suresh, if you feel strongly about removing one of the NamespaceInfo constructor variants, I'd be happy to do that in a follow-up JIRA.
          Aaron T. Myers made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags Reviewed [ 10343 ]
          Fix Version/s 2.0.0 [ 12320353 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2078 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2078/)
          HDFS-3202. NamespaceInfo PB translation drops build version. Contributed by Aaron T. Myers. (Revision 1309955)

          Result = SUCCESS
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309955
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamespaceInfo.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2078 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2078/ ) HDFS-3202 . NamespaceInfo PB translation drops build version. Contributed by Aaron T. Myers. (Revision 1309955) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309955 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamespaceInfo.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2003 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2003/)
          HDFS-3202. NamespaceInfo PB translation drops build version. Contributed by Aaron T. Myers. (Revision 1309955)

          Result = SUCCESS
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309955
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamespaceInfo.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2003 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2003/ ) HDFS-3202 . NamespaceInfo PB translation drops build version. Contributed by Aaron T. Myers. (Revision 1309955) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309955 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamespaceInfo.java
          Hide
          Suresh Srinivas added a comment -

          Suresh, if you feel strongly about removing one of the NamespaceInfo constructor variants, I'd be happy to do that in a follow-up JIRA.

          Just wanted to avoid the many variants of constructors and methods. Our code is replete with this But no issues with the patch though.

          Show
          Suresh Srinivas added a comment - Suresh, if you feel strongly about removing one of the NamespaceInfo constructor variants, I'd be happy to do that in a follow-up JIRA. Just wanted to avoid the many variants of constructors and methods. Our code is replete with this But no issues with the patch though.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2015 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2015/)
          HDFS-3202. NamespaceInfo PB translation drops build version. Contributed by Aaron T. Myers. (Revision 1309955)

          Result = SUCCESS
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309955
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamespaceInfo.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2015 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2015/ ) HDFS-3202 . NamespaceInfo PB translation drops build version. Contributed by Aaron T. Myers. (Revision 1309955) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309955 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamespaceInfo.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1006 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1006/)
          HDFS-3202. NamespaceInfo PB translation drops build version. Contributed by Aaron T. Myers. (Revision 1309955)

          Result = FAILURE
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309955
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamespaceInfo.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1006 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1006/ ) HDFS-3202 . NamespaceInfo PB translation drops build version. Contributed by Aaron T. Myers. (Revision 1309955) Result = FAILURE atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309955 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamespaceInfo.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1041 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1041/)
          HDFS-3202. NamespaceInfo PB translation drops build version. Contributed by Aaron T. Myers. (Revision 1309955)

          Result = SUCCESS
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309955
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamespaceInfo.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1041 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1041/ ) HDFS-3202 . NamespaceInfo PB translation drops build version. Contributed by Aaron T. Myers. (Revision 1309955) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1309955 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamespaceInfo.java

            People

            • Assignee:
              Aaron T. Myers
              Reporter:
              Aaron T. Myers
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development