Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.23.0
    • Component/s: datanode, hdfs-client
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Added header classes for individual DataTransferProtocol op headers.

      Description

      It will make a clear distinction between the variables used in the protocol and the others.

      1. h1966_20110527b.patch
        20 kB
        Tsz Wo Nicholas Sze
      2. h1966_20110526.patch
        4 kB
        Tsz Wo Nicholas Sze
      3. h1966_20110524.patch
        7 kB
        Tsz Wo Nicholas Sze
      4. h1966_20110519.patch
        8 kB
        Tsz Wo Nicholas Sze

        Activity

        Hide
        Tsz Wo Nicholas Sze added a comment -

        h1966_20110519.patch: added CopyBlockHeader for illustrating the idea.

        Show
        Tsz Wo Nicholas Sze added a comment - h1966_20110519.patch: added CopyBlockHeader for illustrating the idea.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        h1966_20110524.patch: reverted some changes.

        Show
        Tsz Wo Nicholas Sze added a comment - h1966_20110524.patch: reverted some changes.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        h1966_20110526.patch: reverted some other changes.

        Show
        Tsz Wo Nicholas Sze added a comment - h1966_20110526.patch: reverted some other changes.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        h1966_20110527b.patch: changed all methods.

        Show
        Tsz Wo Nicholas Sze added a comment - h1966_20110527b.patch: changed all methods.
        Hide
        Hadoop QA added a comment -

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

        +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 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 failed these core unit tests:

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

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/651//testReport/
        Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/651//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/651//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/12480686/h1966_20110527b.patch against trunk revision 1128393. +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 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 failed these core unit tests: +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/651//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/651//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/651//console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        looks like TestFiDataTransferProtocol2 failed. is that due to this patch or just flaky?

        Show
        Todd Lipcon added a comment - looks like TestFiDataTransferProtocol2 failed. is that due to this patch or just flaky?
        Hide
        Tsz Wo Nicholas Sze added a comment -

        TestFiDataTransferProtocol2 is flaky. It is nothing to do with the patch. See HDFS-1923.

        Show
        Tsz Wo Nicholas Sze added a comment - TestFiDataTransferProtocol2 is flaky. It is nothing to do with the patch. See HDFS-1923 .
        Hide
        Jitendra Nath Pandey added a comment -

        1. What is the reason for defining header classes inside the Op enum?
        2. I will recommend adding a factory to create right header object depending on the opcode. The factory could be useful at the receiving end.
        3. Please add a few unit tests for serialization/de-serialization of the headers.

        Show
        Jitendra Nath Pandey added a comment - 1. What is the reason for defining header classes inside the Op enum? 2. I will recommend adding a factory to create right header object depending on the opcode. The factory could be useful at the receiving end. 3. Please add a few unit tests for serialization/de-serialization of the headers.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        Jitendra, thanks for the review.

        > 1. What is the reason for defining header classes inside the Op enum?

        It is because the headers are operation related. There are other classes like PacketHeader which is nothing to do with operations.

        > 2. I will recommend adding a factory to create right header object depending on the opcode. The factory could be useful at the receiving end.

        We already have DataTransferProtocol.Receiver. I think it is the factory you mean.

        > 3. Please add a few unit tests for serialization/de-serialization of the headers.

        We have many tests for read, write, fault-inject tests, balancer, etc. These tests cover DataTransferProtocol. So adding new tests for the header seems redundant. Do you agree?

        Show
        Tsz Wo Nicholas Sze added a comment - Jitendra, thanks for the review. > 1. What is the reason for defining header classes inside the Op enum? It is because the headers are operation related. There are other classes like PacketHeader which is nothing to do with operations. > 2. I will recommend adding a factory to create right header object depending on the opcode. The factory could be useful at the receiving end. We already have DataTransferProtocol.Receiver . I think it is the factory you mean. > 3. Please add a few unit tests for serialization/de-serialization of the headers. We have many tests for read, write, fault-inject tests, balancer, etc. These tests cover DataTransferProtocol . So adding new tests for the header seems redundant. Do you agree?
        Hide
        Jitendra Nath Pandey added a comment -

        I think DataTransferProtocol is getting too cluttered and it might be worthwhile to split it into several classes and interfaces. But that is beyond the scope of this jira.

        +1 for the patch.

        Show
        Jitendra Nath Pandey added a comment - I think DataTransferProtocol is getting too cluttered and it might be worthwhile to split it into several classes and interfaces. But that is beyond the scope of this jira. +1 for the patch.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        I have committed this.

        Show
        Tsz Wo Nicholas Sze added a comment - I have committed this.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #685 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk/685/)
        HDFS-1966. Encapsulate individual DataTransferProtocol op headers.

        szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1130367
        Files :

        • /hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/DataTransferProtocol.java
        • /hadoop/hdfs/trunk/CHANGES.txt
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #685 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk/685/ ) HDFS-1966 . Encapsulate individual DataTransferProtocol op headers. szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1130367 Files : /hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/DataTransferProtocol.java /hadoop/hdfs/trunk/CHANGES.txt
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #704 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/704/)

        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #704 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/704/ )

          People

          • Assignee:
            Tsz Wo Nicholas Sze
            Reporter:
            Tsz Wo Nicholas Sze
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development