Hadoop Common
  1. Hadoop Common
  2. HADOOP-6221

Make it possible to interrupt RPC Client operations

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 0.21.0
    • Fix Version/s: None
    • Component/s: ipc
    • Labels:
      None

      Description

      RPC.waitForProxy swallows any attempts to interrupt it while waiting for a proxy; this makes it hard to shutdown a service that you are starting; you have to wait for the timeouts.

      There are only 4-5 places in the code that use either of the two overloaded methods, removing the catch and changing the signature should not be too painful, unless anyone is using the method outside the hadoop codebase.

      1. HADOOP-6221.patch
        5 kB
        steve_l
      2. HADOOP-6221.patch
        7 kB
        steve_l
      3. HADOOP-6221.patch
        8 kB
        steve_l
      4. HADOOP-6221.patch
        8 kB
        steve_l
      5. HADOOP-6221.patch
        7 kB
        Steve Loughran
      6. HADOOP-6221.patch
        8 kB
        Vinayakumar B

        Issue Links

          Activity

          Hide
          steve_l added a comment -

          Tests show this problem is harder than it appears, even with improved Interrupt support during the sleep(), you can't interrupt the client while it is actually waiting to connect, that gets swallowed somewhere too. The test case shows this; a thread that tries to wait for 20s for a connection, but which is interrupted in another thread never sees the interrupt, instead after 20s it times out and throws the ConnectionRefused fault. What I'd like would be the interrupt acknowledged with the inner exception nested, and for the operation to fail as soon as the thread is interrupted.

          Show
          steve_l added a comment - Tests show this problem is harder than it appears, even with improved Interrupt support during the sleep(), you can't interrupt the client while it is actually waiting to connect, that gets swallowed somewhere too. The test case shows this; a thread that tries to wait for 20s for a connection, but which is interrupted in another thread never sees the interrupt, instead after 20s it times out and throws the ConnectionRefused fault. What I'd like would be the interrupt acknowledged with the inner exception nested, and for the operation to fail as soon as the thread is interrupted.
          Hide
          steve_l added a comment -

          Incidentally, this patch converts the InterruptedException to an InterruptedIOException, therefore the signature of the methods do not change, only their behaviour when interrupted. Programs not interrupting threads that were trying to connect to an unresponsive remote server will not see any change in method signature or behaviour.

          Show
          steve_l added a comment - Incidentally, this patch converts the InterruptedException to an InterruptedIOException, therefore the signature of the methods do not change, only their behaviour when interrupted. Programs not interrupting threads that were trying to connect to an unresponsive remote server will not see any change in method signature or behaviour.
          Hide
          steve_l added a comment -

          the interrupt is being swallowed in Client.call()), which blocks until a call completes or times out

               while (!call.done) {
                  try {
                    call.wait();                           // wait for the result
                  } catch (InterruptedException ie) {
                    // save the fact that we were interrupted
                    interrupted = true;
                  }
                }
          
                if (interrupted) {
                  // set the interrupt flag now that we are done waiting
                  Thread.currentThread().interrupt();
                }
          

          This means that the client thread is blocked until whichever thread is actually doing the RPC. Client.waitForWork() also swallows interrupts.

          This will get complex fast. I think a good tactic would be rather than trying to make the old RPC stack interruptible, focus on making Avro something that you can interrupt, so that going forward you can interrupt client programs trying to talk to unresponsive servers.

          Show
          steve_l added a comment - the interrupt is being swallowed in Client.call()) , which blocks until a call completes or times out while (!call.done) { try { call.wait(); // wait for the result } catch (InterruptedException ie) { // save the fact that we were interrupted interrupted = true ; } } if (interrupted) { // set the interrupt flag now that we are done waiting Thread .currentThread().interrupt(); } This means that the client thread is blocked until whichever thread is actually doing the RPC. Client.waitForWork() also swallows interrupts. This will get complex fast. I think a good tactic would be rather than trying to make the old RPC stack interruptible, focus on making Avro something that you can interrupt, so that going forward you can interrupt client programs trying to talk to unresponsive servers.
          Hide
          steve_l added a comment -

          Retitling this to cover the entire underlying problem: you cannot interrupt a Client if the far end isn't responding.

          Show
          steve_l added a comment - Retitling this to cover the entire underlying problem: you cannot interrupt a Client if the far end isn't responding.
          Hide
          steve_l added a comment -

          This issue benefits from the HADOOP-3457 patch, to the extent that to be able to interrupt RPC operations client-side properly, you need the HADOOP-3457 patch applied.

          I propose merging, as this patch provides the test

          Show
          steve_l added a comment - This issue benefits from the HADOOP-3457 patch, to the extent that to be able to interrupt RPC operations client-side properly, you need the HADOOP-3457 patch applied. I propose merging, as this patch provides the test
          Hide
          steve_l added a comment -

          Updated patch incorporating HADOOP-3457 with the tests passing. There are still no guarantees that the underlying IPC classes can be interrupted but now

          1. the sleeps between attempts to communicate can be interrupted
          2. InterruptedIOExceptions get passed up, so can be filtered on in client code (including tests)
          Show
          steve_l added a comment - Updated patch incorporating HADOOP-3457 with the tests passing. There are still no guarantees that the underlying IPC classes can be interrupted but now the sleeps between attempts to communicate can be interrupted InterruptedIOExceptions get passed up, so can be filtered on in client code (including tests)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12431866/HADOOP-6221.patch
          against trunk revision 904339.

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

          +1 tests included. The patch appears to include 5 new or modified tests.

          +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 generated 2 release audit warnings (more than the trunk's current 1 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-h4.grid.sp2.yahoo.net/304/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/304/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/304/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/304/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/304/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/12431866/HADOOP-6221.patch against trunk revision 904339. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +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 generated 2 release audit warnings (more than the trunk's current 1 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-h4.grid.sp2.yahoo.net/304/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/304/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/304/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/304/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/304/console This message is automatically generated.
          Hide
          steve_l added a comment -

          Updated patch with the ASF header in the test class, which is the probable cause of RAT being unhappy

          Show
          steve_l added a comment - Updated patch with the ASF header in the test class, which is the probable cause of RAT being unhappy
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12434950/HADOOP-6221.patch
          against trunk revision 906388.

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

          +1 tests included. The patch appears to include 5 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/330/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/12434950/HADOOP-6221.patch against trunk revision 906388. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/330/console This message is automatically generated.
          Hide
          steve_l added a comment -

          patch in sync w/ SVN_HEAD

          Show
          steve_l added a comment - patch in sync w/ SVN_HEAD
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12434971/HADOOP-6221.patch
          against trunk revision 906388.

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

          +1 tests included. The patch appears to include 5 new or modified tests.

          +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-h4.grid.sp2.yahoo.net/331/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/331/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/331/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/331/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/12434971/HADOOP-6221.patch against trunk revision 906388. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +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-h4.grid.sp2.yahoo.net/331/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/331/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/331/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/331/console This message is automatically generated.
          Hide
          Steve Loughran added a comment -

          sync up with HEAD

          Show
          Steve Loughran added a comment - sync up with HEAD
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12468404/HADOOP-6221.patch
          against trunk revision 1058881.

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

          +1 tests included. The patch appears to include 2 new or modified tests.

          +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 passed 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://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/182//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/182//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/182//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/12468404/HADOOP-6221.patch against trunk revision 1058881. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +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 passed 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://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/182//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/182//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/182//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12468404/HADOOP-6221.patch
          against trunk revision 1071364.

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

          +1 tests included. The patch appears to include 2 new or modified tests.

          +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 passed 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://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/288//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/288//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/288//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/12468404/HADOOP-6221.patch against trunk revision 1071364. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +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 passed 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://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/288//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/288//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/288//console This message is automatically generated.
          Hide
          Steve Loughran added a comment -

          reworking for the various svn structures

          Show
          Steve Loughran added a comment - reworking for the various svn structures
          Hide
          Vinayakumar B added a comment -

          Attached the patch for trunk version.

          Show
          Vinayakumar B added a comment - Attached the patch for trunk version.

            People

            • Assignee:
              Steve Loughran
              Reporter:
              Steve Loughran
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development