Hadoop Common
  1. Hadoop Common
  2. HADOOP-6132

RPC client opens an extra connection for VersionedProtocol

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: ipc
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Tags:
      RPC

      Description

      RPC client caches connections per protocol. However, since all of our real protocols are subclasses of VersionedProtocol, a bug in the implementation makes the client opens an extra connection just for the VersionedProtocol, which is not needed.

      1. HADOOP-6132-0_20.1.patch
        4 kB
        Jitendra Nath Pandey
      2. 2.patch
        4 kB
        Kan Zhang
      3. 1.patch
        2 kB
        Kan Zhang

        Issue Links

          Activity

          Hide
          Kan Zhang added a comment -

          attaching a patch. Manually verified that the client only opens one connection per protocol.

          Show
          Kan Zhang added a comment - attaching a patch. Manually verified that the client only opens one connection per protocol.
          Hide
          Kan Zhang added a comment -

          added a link to HADOOP-4853. The current patch only fixes the bug identified in this JIRA (in a compatible way). It doesn't address the issue raised in HADOOP-4853.

          Show
          Kan Zhang added a comment - added a link to HADOOP-4853 . The current patch only fixes the bug identified in this JIRA (in a compatible way). It doesn't address the issue raised in HADOOP-4853 .
          Hide
          Owen O'Malley added a comment -

          Shouldn't the protocol be defined as:

          Class<? extends VersionedProtocol> protocol;

          Show
          Owen O'Malley added a comment - Shouldn't the protocol be defined as: Class<? extends VersionedProtocol> protocol;
          Hide
          Kan Zhang added a comment -

          Absolutely. An updated patch attached.

          Show
          Kan Zhang added a comment - Absolutely. An updated patch attached.
          Hide
          Hadoop QA added a comment -

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

          +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 warnings.

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

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/561/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/561/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/561/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/561/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/12412921/2.patch against trunk revision 792812. +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 warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/561/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/561/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/561/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/561/console This message is automatically generated.
          Hide
          Kan Zhang added a comment -

          The manual verification I used was running a test that involves the RPC layer (e.g., a test that uses the MiniDFSCluster), with log level of ipc.Server set to debug. Without this patch, one would observe the following line in the log, which means the client is establishing a connection for the VersionedProtocol. It happens whenever a client sets up the first connection for a protocol (e.g., ClientProtocol or DatanodeProtocol). When the bug is fixed, no connection for the VersionedProtocol should ever be set up. Hence, no such log messages should have been observed in the log.

          2009-07-16 15:41:37,763 DEBUG ipc.Server (Server.java:readAndProcess(861)) - Successfully authorized org.apache.hadoop.ipc.VersionedProtocol-......
          
          Show
          Kan Zhang added a comment - The manual verification I used was running a test that involves the RPC layer (e.g., a test that uses the MiniDFSCluster), with log level of ipc.Server set to debug. Without this patch, one would observe the following line in the log, which means the client is establishing a connection for the VersionedProtocol. It happens whenever a client sets up the first connection for a protocol (e.g., ClientProtocol or DatanodeProtocol). When the bug is fixed, no connection for the VersionedProtocol should ever be set up. Hence, no such log messages should have been observed in the log. 2009-07-16 15:41:37,763 DEBUG ipc.Server (Server.java:readAndProcess(861)) - Successfully authorized org.apache.hadoop.ipc.VersionedProtocol-......
          Hide
          Raghu Angadi added a comment -

          +1 for the fix.

          There is another instance of use of getDelaringClass() in the same file (though that part of the code is not used except in some tests).
          We need to fix that.

          Show
          Raghu Angadi added a comment - +1 for the fix. There is another instance of use of getDelaringClass() in the same file (though that part of the code is not used except in some tests). We need to fix that.
          Hide
          Kan Zhang added a comment -

          > There is another instance of use of getDelaringClass() in the same file

          Yes, it is used in a public static convenience method whereby users can make a call by passing only the method. Unless we want to change the
          interface contract and require the user to pass the protocol as well, we have to use getDeclaringClass() to figure out the protocol. I'm not sure
          what the original purpose of this interface was. But I feel it serves some purpose (as long as users are aware of the twist of using
          getDeclaringClass()) and since it is only used in tests, I don't feel making the changes in this patch. What do you think?

          Show
          Kan Zhang added a comment - > There is another instance of use of getDelaringClass() in the same file Yes, it is used in a public static convenience method whereby users can make a call by passing only the method. Unless we want to change the interface contract and require the user to pass the protocol as well, we have to use getDeclaringClass() to figure out the protocol. I'm not sure what the original purpose of this interface was. But I feel it serves some purpose (as long as users are aware of the twist of using getDeclaringClass()) and since it is only used in tests, I don't feel making the changes in this patch. What do you think?
          Hide
          Raghu Angadi added a comment -

          sure. That is fine. If someone actually ends up using the interface, they will be similarly surprised. But for now I guess it is fine. If we want to fix it, we need to make the interface take class object as one of the arguments (since that is part of definition of what a connection is).

          +1.

          Show
          Raghu Angadi added a comment - sure. That is fine. If someone actually ends up using the interface, they will be similarly surprised. But for now I guess it is fine. If we want to fix it, we need to make the interface take class object as one of the arguments (since that is part of definition of what a connection is). +1.
          Hide
          Raghu Angadi added a comment -

          I just committed this. Thanks Kan.

          Show
          Raghu Angadi added a comment - I just committed this. Thanks Kan.
          Hide
          Jitendra Nath Pandey added a comment -

          Patch for hadoop 20 added.

          Show
          Jitendra Nath Pandey added a comment - Patch for hadoop 20 added.

            People

            • Assignee:
              Kan Zhang
              Reporter:
              Kan Zhang
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development