HBase
  1. HBase
  2. HBASE-3899

enhance HBase RPC to support free-ing up server handler threads even if response is not ready

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.90.6
    • Fix Version/s: 0.92.0
    • Component/s: IPC/RPC
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      In the current implementation, the server handler thread picks up an item from the incoming callqueue, processes it and then wraps the response as a Writable and sends it back to the IPC server module. This wastes thread-resources when the thread is blocked for disk IO (transaction logging, read into block cache, etc).

      It would be nice if we can make the RPC Server Handler threads pick up a call from the IPC queue, hand it over to the application (e.g. HRegion), the application can queue it to be processed asynchronously and send a response back to the IPC server module saying that the response is not ready. The RPC Server Handler thread is now ready to pick up another request from the incoming callqueue. When the queued call is processed by the application, it indicates to the IPC module that the response is now ready to be sent back to the client.

      The RPC client continues to experience the same behaviour as before. A RPC client is synchronous and blocks till the response arrives.

      This RPC enhancement allows us to do very powerful things with the RegionServer. In future, we can make enhance the RegionServer's threading model to a message-passing model for better performance. We will not be limited by the number of threads in the RegionServer.

      1. asyncRpc.txt
        10 kB
        dhruba borthakur
      2. asyncRpc.txt
        10 kB
        dhruba borthakur
      3. HBASE-3899.patch
        16 kB
        Vlad Dogaru
      4. HBASE-3899-2.patch
        21 kB
        Vlad Dogaru
      5. HBASE-3899-amend-v4.patch
        12 kB
        Todd Lipcon

        Activity

        Hide
        dhruba borthakur added a comment -

        This patch allows the RPC handler thread to
        be freed up to process new incoming RPC's while the wal sync is pending.

        The handler thread sends out response only if it has not been responded to earlier. The call.doneResponse field ensures that responses are sent out only once.

        If the application server returns WriteDelayed, then the rpc server does not send out responses to client.

        Show
        dhruba borthakur added a comment - This patch allows the RPC handler thread to be freed up to process new incoming RPC's while the wal sync is pending. The handler thread sends out response only if it has not been responded to earlier. The call.doneResponse field ensures that responses are sent out only once. If the application server returns WriteDelayed, then the rpc server does not send out responses to client.
        Hide
        stack added a comment -

        This patch looks fine to me. Not too invasive. How does it work? Something has to implement WritableDelayed? Thanks D.

        Show
        stack added a comment - This patch looks fine to me. Not too invasive. How does it work? Something has to implement WritableDelayed? Thanks D.
        Hide
        dhruba borthakur added a comment -

        Thanks for the comments stack. The HBase code that will uses it can return WritableDelayed. I have this code but thought it is better to get the ipc code in there first, because it is a standalone piece of code without any depenencies so that the rest of the patch is not bloated. Also, this patch does not change any existing APIs.

        Show
        dhruba borthakur added a comment - Thanks for the comments stack. The HBase code that will uses it can return WritableDelayed. I have this code but thought it is better to get the ipc code in there first, because it is a standalone piece of code without any depenencies so that the rest of the patch is not bloated. Also, this patch does not change any existing APIs.
        Hide
        stack added a comment -

        Committed to TRUNK. Thanks for the patch Dhruba.

        Show
        stack added a comment - Committed to TRUNK. Thanks for the patch Dhruba.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #1939 (See https://builds.apache.org/hudson/job/HBase-TRUNK/1939/)

        Show
        Hudson added a comment - Integrated in HBase-TRUNK #1939 (See https://builds.apache.org/hudson/job/HBase-TRUNK/1939/ )
        Hide
        Gary Helmling added a comment -

        I know this is already committed, but I have some questions about the static methods that were sprinkled in to HBaseServer, and I'd like to better understand what direction we're headed in. In general, the static methods, if invoked outside the RPC layer, break the RpcEngine and RpcServer abstractions that were added to allow loadable RPC engines. There doesn't seem to be anything calling the static methods yet, so pardon if my assumptions are wrong. I assume that's slated for a follow up instead.

        public static void startDelay(Object o) {
          Call call = (Call) o;
          if (call != null) {
            call.startDelay();
          }
        }
        public static void endDelay(Object o) throws IOException {
          Call call = (Call) o;
          if (call != null) {
            call.endDelay();
          }
        }
        

        Since these only invoke the corresponding methods in Call, which are public, why not just invoke those directly?

        public static Object getCall() {
          return CurCall.get();
        }
        

        This will not be valid if an alternate engine/RPC server is loaded. Should we instead have RpcServer.getCall()? Or some other RPC request context?

          static public void processRpcResponse(Object callobj, Writable value,
            String error, String errorClass) throws IOException {
        

        I'm not sure I understand why this one is static, unless it will be invoked throughout other areas of the HBase code? The contents of this method also seem to duplicate what's contained in Handler.run(), so I'm not sure why we have the conditional check in HBaseServer.Handler.run():

        if (!(value instanceof WritableDelayed)) {
          processRpcResponse(call, value, error, errorClass);
        }
        

        It looks to me like both code paths that would be followed are the same? Am I missing something?

        In particular, I rely on the RpcEngine and RpcServer interfaces to be able to load a secure RPC engine transparently. Having HBase code directly using HBaseServer will break that. Can the functionality of these static methods be accommodated instead using instance methods called on a reference to the loaded RpcServer instance?

        I can open a new JIRA to address changes, just wanted to get clarification here first.

        Show
        Gary Helmling added a comment - I know this is already committed, but I have some questions about the static methods that were sprinkled in to HBaseServer, and I'd like to better understand what direction we're headed in. In general, the static methods, if invoked outside the RPC layer, break the RpcEngine and RpcServer abstractions that were added to allow loadable RPC engines. There doesn't seem to be anything calling the static methods yet, so pardon if my assumptions are wrong. I assume that's slated for a follow up instead. public static void startDelay( Object o) { Call call = (Call) o; if (call != null ) { call.startDelay(); } } public static void endDelay( Object o) throws IOException { Call call = (Call) o; if (call != null ) { call.endDelay(); } } Since these only invoke the corresponding methods in Call, which are public, why not just invoke those directly? public static Object getCall() { return CurCall.get(); } This will not be valid if an alternate engine/RPC server is loaded. Should we instead have RpcServer.getCall() ? Or some other RPC request context? static public void processRpcResponse( Object callobj, Writable value, String error, String errorClass) throws IOException { I'm not sure I understand why this one is static, unless it will be invoked throughout other areas of the HBase code? The contents of this method also seem to duplicate what's contained in Handler.run() , so I'm not sure why we have the conditional check in HBaseServer.Handler.run() : if (!(value instanceof WritableDelayed)) { processRpcResponse(call, value, error, errorClass); } It looks to me like both code paths that would be followed are the same? Am I missing something? In particular, I rely on the RpcEngine and RpcServer interfaces to be able to load a secure RPC engine transparently. Having HBase code directly using HBaseServer will break that. Can the functionality of these static methods be accommodated instead using instance methods called on a reference to the loaded RpcServer instance? I can open a new JIRA to address changes, just wanted to get clarification here first.
        Hide
        Andrew Purtell added a comment -

        I can open a new JIRA to address changes, just wanted to get clarification here first.

        +1

        The RPC engine must be fully pluggable to support secure versus non-secure. We can avoid making a hard dependency on security APIs by conditionally building sub-packages. Can't have something breaking that modularity, especially if unused.

        Show
        Andrew Purtell added a comment - I can open a new JIRA to address changes, just wanted to get clarification here first. +1 The RPC engine must be fully pluggable to support secure versus non-secure. We can avoid making a hard dependency on security APIs by conditionally building sub-packages. Can't have something breaking that modularity, especially if unused.
        Hide
        dhruba borthakur added a comment -

        I agree that these calls should probably go through static methods via RpcServer, rather than HBaseServer.

        Show
        dhruba borthakur added a comment - I agree that these calls should probably go through static methods via RpcServer, rather than HBaseServer.
        Hide
        stack added a comment -

        Backed out the patch for now at least until we branch 0.92 (and figure for sure if this is source of "HBASE-3950 IndexOutOfBoundsException reading results")

        Show
        stack added a comment - Backed out the patch for now at least until we branch 0.92 (and figure for sure if this is source of " HBASE-3950 IndexOutOfBoundsException reading results")
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #1952 (See https://builds.apache.org/hudson/job/HBase-TRUNK/1952/)
        HBASE-3899 enhance HBase RPC to support free-ing up server handler threads even if response is not ready – BACKED IT OUT

        stack :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        • /hbase/trunk/CHANGES.txt
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/WritableDelayed.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #1952 (See https://builds.apache.org/hudson/job/HBase-TRUNK/1952/ ) HBASE-3899 enhance HBase RPC to support free-ing up server handler threads even if response is not ready – BACKED IT OUT stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java /hbase/trunk/CHANGES.txt /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/WritableDelayed.java
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1174/
        -----------------------------------------------------------

        Review request for hbase.

        Summary
        -------

        Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba.

        This addresses bug HBASE-3899.
        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs


        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915
        src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e
        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION

        Diff: https://reviews.apache.org/r/1174/diff

        Testing
        -------

        Unit tests run. Also, the patch includes a new unit test.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/ ----------------------------------------------------------- Review request for hbase. Summary ------- Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba. This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915 src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION Diff: https://reviews.apache.org/r/1174/diff Testing ------- Unit tests run. Also, the patch includes a new unit test. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1174/#review1160
        -----------------------------------------------------------

        Can you describe the difference between Dhruba's patch and this one ?
        I assume test suite passed smoothly with this change.

        • Ted

        On 2011-07-21 20:35:22, Vlad Dogaru wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1174/

        -----------------------------------------------------------

        (Updated 2011-07-21 20:35:22)

        Review request for hbase.

        Summary

        -------

        Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba.

        This addresses bug HBASE-3899.

        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915

        src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION

        Diff: https://reviews.apache.org/r/1174/diff

        Testing

        -------

        Unit tests run. Also, the patch includes a new unit test.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/#review1160 ----------------------------------------------------------- Can you describe the difference between Dhruba's patch and this one ? I assume test suite passed smoothly with this change. Ted On 2011-07-21 20:35:22, Vlad Dogaru wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/ ----------------------------------------------------------- (Updated 2011-07-21 20:35:22) Review request for hbase. Summary ------- Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba. This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915 src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION Diff: https://reviews.apache.org/r/1174/diff Testing ------- Unit tests run. Also, the patch includes a new unit test. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1174/#review1159
        -----------------------------------------------------------

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/1174/#comment2399>

        are these two booleans totally orthogonal? or should we really have a tri-state enum? [not sure yet]

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java
        <https://reviews.apache.org/r/1174/#comment2400>

        this test seems pretty timing-dependent. I wouldn't trust that the sleeps are enough to force the order you expect

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java
        <https://reviews.apache.org/r/1174/#comment2401>

        I don't follow. Isn't the idea here that the rpc handler would return an unused value immediately (eg return -1) and then another thread would later set the return value?

        i.e I'd have expected the function implementatoin to be something like:

        public int test(boolean delay) {
        if (!delay)

        { return UNDELAYED; }

        final Delayable call = rpcServer.getCurrentCall();
        call.startDelay();
        new Thread() {
        public void run()

        { call.endDelay(DELAYED); }

        }
        return 0xDEADBEEF; // this return value should not go back to client
        }

        what am I missing?

        • Todd

        On 2011-07-21 20:35:22, Vlad Dogaru wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1174/

        -----------------------------------------------------------

        (Updated 2011-07-21 20:35:22)

        Review request for hbase.

        Summary

        -------

        Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba.

        This addresses bug HBASE-3899.

        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915

        src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION

        Diff: https://reviews.apache.org/r/1174/diff

        Testing

        -------

        Unit tests run. Also, the patch includes a new unit test.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/#review1159 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/1174/#comment2399 > are these two booleans totally orthogonal? or should we really have a tri-state enum? [not sure yet] src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java < https://reviews.apache.org/r/1174/#comment2400 > this test seems pretty timing-dependent. I wouldn't trust that the sleeps are enough to force the order you expect src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java < https://reviews.apache.org/r/1174/#comment2401 > I don't follow. Isn't the idea here that the rpc handler would return an unused value immediately (eg return -1) and then another thread would later set the return value? i.e I'd have expected the function implementatoin to be something like: public int test(boolean delay) { if (!delay) { return UNDELAYED; } final Delayable call = rpcServer.getCurrentCall(); call.startDelay(); new Thread() { public void run() { call.endDelay(DELAYED); } } return 0xDEADBEEF; // this return value should not go back to client } what am I missing? Todd On 2011-07-21 20:35:22, Vlad Dogaru wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/ ----------------------------------------------------------- (Updated 2011-07-21 20:35:22) Review request for hbase. Summary ------- Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba. This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915 src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION Diff: https://reviews.apache.org/r/1174/diff Testing ------- Unit tests run. Also, the patch includes a new unit test. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-07-21 21:31:37, Ted Yu wrote:

        > Can you describe the difference between Dhruba's patch and this one ?

        > I assume test suite passed smoothly with this change.

        Dhruba's patch added static methods in HBaseServer, which meant that the RPC engine was no longer pluggable, as noted in this comment: https://issues.apache.org/jira/browse/HBASE-3899?focusedCommentId=13040035&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13040035

        This patch adds an interface (Delayable) for delayable remote calls and a new method (getCurrentCall) in the RpcServer interface, so there is no direct dependecy to HBaseServer.

        The test suite passed, but delayed RPCs are not used anywhere in the code yet, I will follow up with more patches.

        • Vlad

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1174/#review1160
        -----------------------------------------------------------

        On 2011-07-21 20:35:22, Vlad Dogaru wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1174/

        -----------------------------------------------------------

        (Updated 2011-07-21 20:35:22)

        Review request for hbase.

        Summary

        -------

        Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba.

        This addresses bug HBASE-3899.

        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915

        src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION

        Diff: https://reviews.apache.org/r/1174/diff

        Testing

        -------

        Unit tests run. Also, the patch includes a new unit test.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-07-21 21:31:37, Ted Yu wrote: > Can you describe the difference between Dhruba's patch and this one ? > I assume test suite passed smoothly with this change. Dhruba's patch added static methods in HBaseServer, which meant that the RPC engine was no longer pluggable, as noted in this comment: https://issues.apache.org/jira/browse/HBASE-3899?focusedCommentId=13040035&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13040035 This patch adds an interface (Delayable) for delayable remote calls and a new method (getCurrentCall) in the RpcServer interface, so there is no direct dependecy to HBaseServer. The test suite passed, but delayed RPCs are not used anywhere in the code yet, I will follow up with more patches. Vlad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/#review1160 ----------------------------------------------------------- On 2011-07-21 20:35:22, Vlad Dogaru wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/ ----------------------------------------------------------- (Updated 2011-07-21 20:35:22) Review request for hbase. Summary ------- Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba. This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915 src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION Diff: https://reviews.apache.org/r/1174/diff Testing ------- Unit tests run. Also, the patch includes a new unit test. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1174/
        -----------------------------------------------------------

        (Updated 2011-07-22 00:17:13.632145)

        Review request for hbase.

        Summary
        -------

        Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba.

        This addresses bug HBASE-3899.
        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs (updated)


        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915
        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION

        Diff: https://reviews.apache.org/r/1174/diff

        Testing
        -------

        Unit tests run. Also, the patch includes a new unit test.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/ ----------------------------------------------------------- (Updated 2011-07-22 00:17:13.632145) Review request for hbase. Summary ------- Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba. This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs (updated) src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915 src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION Diff: https://reviews.apache.org/r/1174/diff Testing ------- Unit tests run. Also, the patch includes a new unit test. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-07-21 21:46:40, Todd Lipcon wrote:

        > src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, lines 244-245

        > <https://reviews.apache.org/r/1174/diff/1/?file=26654#file26654line244>

        >

        > are these two booleans totally orthogonal? or should we really have a tri-state enum? [not sure yet]

        Yes, the response is send as soon as endDelay is called (I changed the API).

        On 2011-07-21 21:46:40, Todd Lipcon wrote:

        > src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java, lines 72-74

        > <https://reviews.apache.org/r/1174/diff/1/?file=26656#file26656line72>

        >

        > this test seems pretty timing-dependent. I wouldn't trust that the sleeps are enough to force the order you expect

        I am not sure how otherwise to enforce the order. Do you have any suggestions?

        On 2011-07-21 21:46:40, Todd Lipcon wrote:

        > src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java, lines 87-98

        > <https://reviews.apache.org/r/1174/diff/1/?file=26656#file26656line87>

        >

        > I don't follow. Isn't the idea here that the rpc handler would return an unused value immediately (eg return -1) and then another thread would later set the return value?

        >

        > i.e I'd have expected the function implementatoin to be something like:

        >

        > public int test(boolean delay) {

        > if (!delay) { return UNDELAYED; }

        > final Delayable call = rpcServer.getCurrentCall();

        > call.startDelay();

        > new Thread() {

        > public void run() { bq. > call.endDelay(DELAYED); bq. > }

        > }

        > return 0xDEADBEEF; // this return value should not go back to client

        > }

        >

        > what am I missing?

        Ah, I was overlooking setting the return value. The endDelay function now takes a Writable as its parameter. Will upload a new patch.

        • Vlad

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1174/#review1159
        -----------------------------------------------------------

        On 2011-07-22 00:17:13, Vlad Dogaru wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1174/

        -----------------------------------------------------------

        (Updated 2011-07-22 00:17:13)

        Review request for hbase.

        Summary

        -------

        Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba.

        This addresses bug HBASE-3899.

        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs

        -----

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION

        Diff: https://reviews.apache.org/r/1174/diff

        Testing

        -------

        Unit tests run. Also, the patch includes a new unit test.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-07-21 21:46:40, Todd Lipcon wrote: > src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, lines 244-245 > < https://reviews.apache.org/r/1174/diff/1/?file=26654#file26654line244 > > > are these two booleans totally orthogonal? or should we really have a tri-state enum? [not sure yet] Yes, the response is send as soon as endDelay is called (I changed the API). On 2011-07-21 21:46:40, Todd Lipcon wrote: > src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java, lines 72-74 > < https://reviews.apache.org/r/1174/diff/1/?file=26656#file26656line72 > > > this test seems pretty timing-dependent. I wouldn't trust that the sleeps are enough to force the order you expect I am not sure how otherwise to enforce the order. Do you have any suggestions? On 2011-07-21 21:46:40, Todd Lipcon wrote: > src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java, lines 87-98 > < https://reviews.apache.org/r/1174/diff/1/?file=26656#file26656line87 > > > I don't follow. Isn't the idea here that the rpc handler would return an unused value immediately (eg return -1) and then another thread would later set the return value? > > i.e I'd have expected the function implementatoin to be something like: > > public int test(boolean delay) { > if (!delay) { return UNDELAYED; } > final Delayable call = rpcServer.getCurrentCall(); > call.startDelay(); > new Thread() { > public void run() { bq. > call.endDelay(DELAYED); bq. > } > } > return 0xDEADBEEF; // this return value should not go back to client > } > > what am I missing? Ah, I was overlooking setting the return value. The endDelay function now takes a Writable as its parameter. Will upload a new patch. Vlad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/#review1159 ----------------------------------------------------------- On 2011-07-22 00:17:13, Vlad Dogaru wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/ ----------------------------------------------------------- (Updated 2011-07-22 00:17:13) Review request for hbase. Summary ------- Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba. This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs ----- src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915 src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION Diff: https://reviews.apache.org/r/1174/diff Testing ------- Unit tests run. Also, the patch includes a new unit test. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1174/#review1175
        -----------------------------------------------------------

        Ship it!

        Looks good to me. Small items below. Have you run this code on a cluster under load?

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/1174/#comment2464>

        Why remove the static? What in the outer class do we need in here in Call?

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/1174/#comment2465>

        Any chance that this synchronized slows down rpc'ing?

        It doesn't look too bad though. We're just allocating and then copying into the allocation the response.

        • Michael

        On 2011-07-22 00:17:13, Vlad Dogaru wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1174/

        -----------------------------------------------------------

        (Updated 2011-07-22 00:17:13)

        Review request for hbase.

        Summary

        -------

        Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba.

        This addresses bug HBASE-3899.

        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs

        -----

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION

        Diff: https://reviews.apache.org/r/1174/diff

        Testing

        -------

        Unit tests run. Also, the patch includes a new unit test.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/#review1175 ----------------------------------------------------------- Ship it! Looks good to me. Small items below. Have you run this code on a cluster under load? src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/1174/#comment2464 > Why remove the static? What in the outer class do we need in here in Call? src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/1174/#comment2465 > Any chance that this synchronized slows down rpc'ing? It doesn't look too bad though. We're just allocating and then copying into the allocation the response. Michael On 2011-07-22 00:17:13, Vlad Dogaru wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/ ----------------------------------------------------------- (Updated 2011-07-22 00:17:13) Review request for hbase. Summary ------- Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba. This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs ----- src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915 src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION Diff: https://reviews.apache.org/r/1174/diff Testing ------- Unit tests run. Also, the patch includes a new unit test. Thanks, Vlad
        Hide
        Vlad Dogaru added a comment -

        @stack, followup from review board: HBaseServer.Call uses warnResponseSize from parent class.

        Also, similar code is in production on Facebook clusters. This patch only adds and tests new behavior, but it is not actually used yet.

        Show
        Vlad Dogaru added a comment - @stack, followup from review board: HBaseServer.Call uses warnResponseSize from parent class. Also, similar code is in production on Facebook clusters. This patch only adds and tests new behavior, but it is not actually used yet.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1174/#review1179
        -----------------------------------------------------------

        I think we need some additional metrics for number of outstanding (delayed) calls... how do we debug cases where calls are getting orphaned?

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java
        <https://reviews.apache.org/r/1174/#comment2475>

        RPC calls can return Writables or any java primitive supported by ObjectWritable.

        So, this should probably be Object result.

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/1174/#comment2476>

        this isn't your code... but this expression is always true!

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/1174/#comment2477>

        this is a no-op. need proper error handling

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/1174/#comment2478>

        assert this.delayResponse

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/1174/#comment2479>

        assert !delayResponse

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/1174/#comment2480>

        if !delayResponse, would we ever have response == null?

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/1174/#comment2481>

        shouldn't this just be a call to enqueueInSelector now?

        • Todd

        On 2011-07-22 00:17:13, Vlad Dogaru wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1174/

        -----------------------------------------------------------

        (Updated 2011-07-22 00:17:13)

        Review request for hbase.

        Summary

        -------

        Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba.

        This addresses bug HBASE-3899.

        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs

        -----

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION

        Diff: https://reviews.apache.org/r/1174/diff

        Testing

        -------

        Unit tests run. Also, the patch includes a new unit test.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/#review1179 ----------------------------------------------------------- I think we need some additional metrics for number of outstanding (delayed) calls... how do we debug cases where calls are getting orphaned? src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java < https://reviews.apache.org/r/1174/#comment2475 > RPC calls can return Writables or any java primitive supported by ObjectWritable. So, this should probably be Object result. src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/1174/#comment2476 > this isn't your code... but this expression is always true! src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/1174/#comment2477 > this is a no-op. need proper error handling src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/1174/#comment2478 > assert this.delayResponse src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/1174/#comment2479 > assert !delayResponse src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/1174/#comment2480 > if !delayResponse, would we ever have response == null? src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/1174/#comment2481 > shouldn't this just be a call to enqueueInSelector now? Todd On 2011-07-22 00:17:13, Vlad Dogaru wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/ ----------------------------------------------------------- (Updated 2011-07-22 00:17:13) Review request for hbase. Summary ------- Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba. This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs ----- src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915 src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION Diff: https://reviews.apache.org/r/1174/diff Testing ------- Unit tests run. Also, the patch includes a new unit test. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1174/
        -----------------------------------------------------------

        (Updated 2011-07-26 01:19:52.655737)

        Review request for hbase.

        Changes
        -------

        • Add checking for number of calls currently delayed. A warning message is issued if too many calls are delayed.
        • Unit test to check that above warning works.
        • endDelay() now takes an Object as a parameter, not a Writable. Initially, I thought the method that ended the delay should pack the response (i.e. endDelay(new HbaseObjectWritable(retval))), but it makes more sense to pack it in setResponse.
        • Address other feedback from Todd Lipcon. Thanks!

        Summary
        -------

        Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba.

        This addresses bug HBASE-3899.
        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs (updated)


        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915
        src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e
        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION

        Diff: https://reviews.apache.org/r/1174/diff

        Testing
        -------

        Unit tests run. Also, the patch includes a new unit test.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/ ----------------------------------------------------------- (Updated 2011-07-26 01:19:52.655737) Review request for hbase. Changes ------- Add checking for number of calls currently delayed. A warning message is issued if too many calls are delayed. Unit test to check that above warning works. endDelay() now takes an Object as a parameter, not a Writable. Initially, I thought the method that ended the delay should pack the response (i.e. endDelay(new HbaseObjectWritable(retval))), but it makes more sense to pack it in setResponse. Address other feedback from Todd Lipcon. Thanks! Summary ------- Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba. This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs (updated) src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915 src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION Diff: https://reviews.apache.org/r/1174/diff Testing ------- Unit tests run. Also, the patch includes a new unit test. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1174/#review1185
        -----------------------------------------------------------

        Ship it!

        Looks good. Have you run the full test suite with the current iteration of the patch?

        • Todd

        On 2011-07-26 01:19:52, Vlad Dogaru wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1174/

        -----------------------------------------------------------

        (Updated 2011-07-26 01:19:52)

        Review request for hbase.

        Summary

        -------

        Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba.

        This addresses bug HBASE-3899.

        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915

        src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION

        Diff: https://reviews.apache.org/r/1174/diff

        Testing

        -------

        Unit tests run. Also, the patch includes a new unit test.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/#review1185 ----------------------------------------------------------- Ship it! Looks good. Have you run the full test suite with the current iteration of the patch? Todd On 2011-07-26 01:19:52, Vlad Dogaru wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/ ----------------------------------------------------------- (Updated 2011-07-26 01:19:52) Review request for hbase. Summary ------- Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba. This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915 src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION Diff: https://reviews.apache.org/r/1174/diff Testing ------- Unit tests run. Also, the patch includes a new unit test. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-07-26 01:38:40, Todd Lipcon wrote:

        > Looks good. Have you run the full test suite with the current iteration of the patch?

        Yes, tests are fine with the current patch.

        • Vlad

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1174/#review1185
        -----------------------------------------------------------

        On 2011-07-26 01:19:52, Vlad Dogaru wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1174/

        -----------------------------------------------------------

        (Updated 2011-07-26 01:19:52)

        Review request for hbase.

        Summary

        -------

        Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba.

        This addresses bug HBASE-3899.

        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915

        src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION

        Diff: https://reviews.apache.org/r/1174/diff

        Testing

        -------

        Unit tests run. Also, the patch includes a new unit test.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-07-26 01:38:40, Todd Lipcon wrote: > Looks good. Have you run the full test suite with the current iteration of the patch? Yes, tests are fine with the current patch. Vlad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/#review1185 ----------------------------------------------------------- On 2011-07-26 01:19:52, Vlad Dogaru wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1174/ ----------------------------------------------------------- (Updated 2011-07-26 01:19:52) Review request for hbase. Summary ------- Free up RPC server Handler thread if the called routine specifies the call should be delayed. The RPC client sees no difference, changes are server-side only. This is based on the previous submitted patch from Dhruba. This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 61d3915 src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java 0da7f9e src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java PRE-CREATION Diff: https://reviews.apache.org/r/1174/diff Testing ------- Unit tests run. Also, the patch includes a new unit test. Thanks, Vlad
        Hide
        Vlad Dogaru added a comment -

        Latest patch form review board.

        Show
        Vlad Dogaru added a comment - Latest patch form review board.
        Hide
        stack added a comment -

        Applied to TRUNK. Thanks for the patch Vlad (and to the reviewers)

        Show
        stack added a comment - Applied to TRUNK. Thanks for the patch Vlad (and to the reviewers)
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2053 (See https://builds.apache.org/job/HBase-TRUNK/2053/)
        HBASE-3899 enhance HBase RPC to support free-ing up server handler threads even if response is not ready

        stack :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        • /hbase/trunk/CHANGES.txt
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/ipc
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2053 (See https://builds.apache.org/job/HBase-TRUNK/2053/ ) HBASE-3899 enhance HBase RPC to support free-ing up server handler threads even if response is not ready stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java /hbase/trunk/CHANGES.txt /hbase/trunk/src/test/java/org/apache/hadoop/hbase/ipc /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
        Hide
        stack added a comment -

        Vlad, I have a test failing after applying this. Does it fail for you?

        Running org.apache.hadoop.hbase.master.TestHMasterRPCException
        Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.716 sec <<< FAILURE!
        

        Thanks.

        Show
        stack added a comment - Vlad, I have a test failing after applying this. Does it fail for you? Running org.apache.hadoop.hbase.master.TestHMasterRPCException Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.716 sec <<< FAILURE! Thanks.
        Hide
        Jonathan Gray added a comment -

        Test passes for me on trunk.

        Show
        Jonathan Gray added a comment - Test passes for me on trunk.
        Hide
        Vlad Dogaru added a comment -

        Just ran the test a few times, it passed each time.

        Show
        Vlad Dogaru added a comment - Just ran the test a few times, it passed each time.
        Hide
        stack added a comment -

        @Vlad (& Jon) Sorry about the false alarm... Passes for me to now (and more importantly up on jenkins). Sorry about that.

        Show
        stack added a comment - @Vlad (& Jon) Sorry about the false alarm... Passes for me to now (and more importantly up on jenkins). Sorry about that.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1257/
        -----------------------------------------------------------

        Review request for hbase.

        Summary
        -------

        Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes:

        • change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay;
        • make HBaseServer work according to the new semantics;
        • add a unit test. We now have testDelayedRpc {Immediate,Delayed}

          Response, testing that immediate and delayed return values work.

        Please review, thanks!

        This addresses bug HBASE-3899.
        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs


        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae
        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf

        Diff: https://reviews.apache.org/r/1257/diff

        Testing
        -------

        Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/ ----------------------------------------------------------- Review request for hbase. Summary ------- Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes: change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay; make HBaseServer work according to the new semantics; add a unit test. We now have testDelayedRpc {Immediate,Delayed} Response, testing that immediate and delayed return values work. Please review, thanks! This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf Diff: https://reviews.apache.org/r/1257/diff Testing ------- Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1257/#review1266
        -----------------------------------------------------------

        Seems reasonable. I would advocate using the term "returnValue" rather than "response" here for clarity. In both cases, the response is delayed, it's just that the return value hasn't been set yet in one of the cases.

        • Todd

        On 2011-08-02 17:54:13, Vlad Dogaru wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1257/

        -----------------------------------------------------------

        (Updated 2011-08-02 17:54:13)

        Review request for hbase.

        Summary

        -------

        Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes:

        * change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay;

        * make HBaseServer work according to the new semantics;

        * add a unit test. We now have testDelayedRpc{Immediate,Delayed}Response, testing that immediate and delayed return values work.

        Please review, thanks!

        This addresses bug HBASE-3899.

        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf

        Diff: https://reviews.apache.org/r/1257/diff

        Testing

        -------

        Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/#review1266 ----------------------------------------------------------- Seems reasonable. I would advocate using the term "returnValue" rather than "response" here for clarity. In both cases, the response is delayed, it's just that the return value hasn't been set yet in one of the cases. Todd On 2011-08-02 17:54:13, Vlad Dogaru wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/ ----------------------------------------------------------- (Updated 2011-08-02 17:54:13) Review request for hbase. Summary ------- Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes: * change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay; * make HBaseServer work according to the new semantics; * add a unit test. We now have testDelayedRpc{Immediate,Delayed}Response, testing that immediate and delayed return values work. Please review, thanks! This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf Diff: https://reviews.apache.org/r/1257/diff Testing ------- Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-08-02 19:00:45, Todd Lipcon wrote:

        > Seems reasonable. I would advocate using the term "returnValue" rather than "response" here for clarity. In both cases, the response is delayed, it's just that the return value hasn't been set yet in one of the cases.

        I agree. I think this would be clearer and easier to read through with s/response/result/ or the like.

        • Gary

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1257/#review1266
        -----------------------------------------------------------

        On 2011-08-02 17:54:13, Vlad Dogaru wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1257/

        -----------------------------------------------------------

        (Updated 2011-08-02 17:54:13)

        Review request for hbase.

        Summary

        -------

        Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes:

        * change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay;

        * make HBaseServer work according to the new semantics;

        * add a unit test. We now have testDelayedRpc{Immediate,Delayed}Response, testing that immediate and delayed return values work.

        Please review, thanks!

        This addresses bug HBASE-3899.

        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf

        Diff: https://reviews.apache.org/r/1257/diff

        Testing

        -------

        Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-08-02 19:00:45, Todd Lipcon wrote: > Seems reasonable. I would advocate using the term "returnValue" rather than "response" here for clarity. In both cases, the response is delayed, it's just that the return value hasn't been set yet in one of the cases. I agree. I think this would be clearer and easier to read through with s/response/result/ or the like. Gary ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/#review1266 ----------------------------------------------------------- On 2011-08-02 17:54:13, Vlad Dogaru wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/ ----------------------------------------------------------- (Updated 2011-08-02 17:54:13) Review request for hbase. Summary ------- Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes: * change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay; * make HBaseServer work according to the new semantics; * add a unit test. We now have testDelayedRpc{Immediate,Delayed}Response, testing that immediate and delayed return values work. Please review, thanks! This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf Diff: https://reviews.apache.org/r/1257/diff Testing ------- Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1257/
        -----------------------------------------------------------

        (Updated 2011-08-02 20:00:02.348642)

        Review request for hbase.

        Changes
        -------

        Todd, Gary, thanks for the review. I agree that returnValue is less likely to cause confusion. Summary of what changed in second iteration:

        • change 'response' to 'returnValue' where appropriate;
        • 'isReturnValueDelayed' is now a method in the Delayable interface – if all Delayables take the boolean parameter, all of them should expose this method;
        • boolean parameter has the same semantics in all calls – previously it was 'isRetValDelayed' and 'setRetValImmediately', which wasn't really a good idea

        Summary
        -------

        Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes:

        • change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay;
        • make HBaseServer work according to the new semantics;
        • add a unit test. We now have testDelayedRpc {Immediate,Delayed}

          Response, testing that immediate and delayed return values work.

        Please review, thanks!

        This addresses bug HBASE-3899.
        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs (updated)


        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae
        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf

        Diff: https://reviews.apache.org/r/1257/diff

        Testing
        -------

        Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/ ----------------------------------------------------------- (Updated 2011-08-02 20:00:02.348642) Review request for hbase. Changes ------- Todd, Gary, thanks for the review. I agree that returnValue is less likely to cause confusion. Summary of what changed in second iteration: change 'response' to 'returnValue' where appropriate; 'isReturnValueDelayed' is now a method in the Delayable interface – if all Delayables take the boolean parameter, all of them should expose this method; boolean parameter has the same semantics in all calls – previously it was 'isRetValDelayed' and 'setRetValImmediately', which wasn't really a good idea Summary ------- Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes: change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay; make HBaseServer work according to the new semantics; add a unit test. We now have testDelayedRpc {Immediate,Delayed} Response, testing that immediate and delayed return values work. Please review, thanks! This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs (updated) src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf Diff: https://reviews.apache.org/r/1257/diff Testing ------- Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1257/#review1297
        -----------------------------------------------------------

        This looks good to me.

        Only remaining question I have is this: what happens if the response we want to eventually make is an exception? I think we should also have an endDelayThrowing(Throwable t) perhaps? It seems this is a likely use case: you delay a call because you're waiting to get some lock, then only once you get that lock, you find that some error occurs.

        • Todd

        On 2011-08-02 20:00:02, Vlad Dogaru wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1257/

        -----------------------------------------------------------

        (Updated 2011-08-02 20:00:02)

        Review request for hbase.

        Summary

        -------

        Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes:

        * change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay;

        * make HBaseServer work according to the new semantics;

        * add a unit test. We now have testDelayedRpc{Immediate,Delayed}Response, testing that immediate and delayed return values work.

        Please review, thanks!

        This addresses bug HBASE-3899.

        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf

        Diff: https://reviews.apache.org/r/1257/diff

        Testing

        -------

        Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/#review1297 ----------------------------------------------------------- This looks good to me. Only remaining question I have is this: what happens if the response we want to eventually make is an exception? I think we should also have an endDelayThrowing(Throwable t) perhaps? It seems this is a likely use case: you delay a call because you're waiting to get some lock, then only once you get that lock, you find that some error occurs. Todd On 2011-08-02 20:00:02, Vlad Dogaru wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/ ----------------------------------------------------------- (Updated 2011-08-02 20:00:02) Review request for hbase. Summary ------- Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes: * change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay; * make HBaseServer work according to the new semantics; * add a unit test. We now have testDelayedRpc{Immediate,Delayed}Response, testing that immediate and delayed return values work. Please review, thanks! This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf Diff: https://reviews.apache.org/r/1257/diff Testing ------- Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1257/
        -----------------------------------------------------------

        (Updated 2011-08-05 00:13:01.552698)

        Review request for hbase.

        Changes
        -------

        Add endDelayThrowing and a unit test to cover it, thanks for the suggestion.

        Summary
        -------

        Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes:

        • change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay;
        • make HBaseServer work according to the new semantics;
        • add a unit test. We now have testDelayedRpc {Immediate,Delayed}

          Response, testing that immediate and delayed return values work.

        Please review, thanks!

        This addresses bug HBASE-3899.
        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs (updated)


        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae
        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf

        Diff: https://reviews.apache.org/r/1257/diff

        Testing
        -------

        Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/ ----------------------------------------------------------- (Updated 2011-08-05 00:13:01.552698) Review request for hbase. Changes ------- Add endDelayThrowing and a unit test to cover it, thanks for the suggestion. Summary ------- Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes: change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay; make HBaseServer work according to the new semantics; add a unit test. We now have testDelayedRpc {Immediate,Delayed} Response, testing that immediate and delayed return values work. Please review, thanks! This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs (updated) src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf Diff: https://reviews.apache.org/r/1257/diff Testing ------- Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1257/#review1298
        -----------------------------------------------------------

        Ship it!

        Looks good. I'll let Gary take a look as well before commit.

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/1257/#comment2952>

        assert this.delayResponse here, right?

        • Todd

        On 2011-08-05 00:13:01, Vlad Dogaru wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1257/

        -----------------------------------------------------------

        (Updated 2011-08-05 00:13:01)

        Review request for hbase.

        Summary

        -------

        Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes:

        * change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay;

        * make HBaseServer work according to the new semantics;

        * add a unit test. We now have testDelayedRpc{Immediate,Delayed}Response, testing that immediate and delayed return values work.

        Please review, thanks!

        This addresses bug HBASE-3899.

        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf

        Diff: https://reviews.apache.org/r/1257/diff

        Testing

        -------

        Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/#review1298 ----------------------------------------------------------- Ship it! Looks good. I'll let Gary take a look as well before commit. src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/1257/#comment2952 > assert this.delayResponse here, right? Todd On 2011-08-05 00:13:01, Vlad Dogaru wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/ ----------------------------------------------------------- (Updated 2011-08-05 00:13:01) Review request for hbase. Summary ------- Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes: * change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay; * make HBaseServer work according to the new semantics; * add a unit test. We now have testDelayedRpc{Immediate,Delayed}Response, testing that immediate and delayed return values work. Please review, thanks! This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf Diff: https://reviews.apache.org/r/1257/diff Testing ------- Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1257/#review1299
        -----------------------------------------------------------

        Ship it!

        +1 Looks good to me.

        Might be worth adding a convenience method to Call (for when not using delayReturnValue)

        public synchronized void endDelay() throws IOException

        { endDelay(null); }

        to avoid having to understand what the null means from calling code? But on the other hand that's another method to add to the interface and not so important. So I don't have a problem leaving that out either. You guys have been the ones using this so if you don't see the need, fine by me.

        • Gary

        On 2011-08-05 00:13:01, Vlad Dogaru wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1257/

        -----------------------------------------------------------

        (Updated 2011-08-05 00:13:01)

        Review request for hbase.

        Summary

        -------

        Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes:

        * change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay;

        * make HBaseServer work according to the new semantics;

        * add a unit test. We now have testDelayedRpc{Immediate,Delayed}Response, testing that immediate and delayed return values work.

        Please review, thanks!

        This addresses bug HBASE-3899.

        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf

        Diff: https://reviews.apache.org/r/1257/diff

        Testing

        -------

        Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/#review1299 ----------------------------------------------------------- Ship it! +1 Looks good to me. Might be worth adding a convenience method to Call (for when not using delayReturnValue) public synchronized void endDelay() throws IOException { endDelay(null); } to avoid having to understand what the null means from calling code? But on the other hand that's another method to add to the interface and not so important. So I don't have a problem leaving that out either. You guys have been the ones using this so if you don't see the need, fine by me. Gary On 2011-08-05 00:13:01, Vlad Dogaru wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/ ----------------------------------------------------------- (Updated 2011-08-05 00:13:01) Review request for hbase. Summary ------- Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes: * change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay; * make HBaseServer work according to the new semantics; * add a unit test. We now have testDelayedRpc{Immediate,Delayed}Response, testing that immediate and delayed return values work. Please review, thanks! This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf Diff: https://reviews.apache.org/r/1257/diff Testing ------- Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1257/
        -----------------------------------------------------------

        (Updated 2011-08-05 17:59:06.111216)

        Review request for hbase.

        Changes
        -------

        Add endDelay() to the interface, for use when the return value was not delayed. Thanks for the suggestion.

        Summary
        -------

        Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes:

        • change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay;
        • make HBaseServer work according to the new semantics;
        • add a unit test. We now have testDelayedRpc {Immediate,Delayed}

          Response, testing that immediate and delayed return values work.

        Please review, thanks!

        This addresses bug HBASE-3899.
        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs (updated)


        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae
        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf

        Diff: https://reviews.apache.org/r/1257/diff

        Testing
        -------

        Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/ ----------------------------------------------------------- (Updated 2011-08-05 17:59:06.111216) Review request for hbase. Changes ------- Add endDelay() to the interface, for use when the return value was not delayed. Thanks for the suggestion. Summary ------- Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes: change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay; make HBaseServer work according to the new semantics; add a unit test. We now have testDelayedRpc {Immediate,Delayed} Response, testing that immediate and delayed return values work. Please review, thanks! This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs (updated) src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf Diff: https://reviews.apache.org/r/1257/diff Testing ------- Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak. Thanks, Vlad
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1257/#review1302
        -----------------------------------------------------------

        Ship it!

        +1 from me

        Todd you going to commit?

        • Gary

        On 2011-08-05 17:59:06, Vlad Dogaru wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1257/

        -----------------------------------------------------------

        (Updated 2011-08-05 17:59:06)

        Review request for hbase.

        Summary

        -------

        Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes:

        * change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay;

        * make HBaseServer work according to the new semantics;

        * add a unit test. We now have testDelayedRpc{Immediate,Delayed}Response, testing that immediate and delayed return values work.

        Please review, thanks!

        This addresses bug HBASE-3899.

        https://issues.apache.org/jira/browse/HBASE-3899

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae

        src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf

        Diff: https://reviews.apache.org/r/1257/diff

        Testing

        -------

        Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak.

        Thanks,

        Vlad

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/#review1302 ----------------------------------------------------------- Ship it! +1 from me Todd you going to commit? Gary On 2011-08-05 17:59:06, Vlad Dogaru wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1257/ ----------------------------------------------------------- (Updated 2011-08-05 17:59:06) Review request for hbase. Summary ------- Some use cases of delayed RPCs need the ability to delay the call until a further moment, but set the return value immediately. For example, if a handler calls a function returning void that might delay (a sync), the result does not depend on the function's result (since it has none). This patch enables that by making the following changes: * change the Delayable interface so that startDelay specifies whether the return value should be set immediately after call return or at the end of the delay; * make HBaseServer work according to the new semantics; * add a unit test. We now have testDelayedRpc{Immediate,Delayed}Response, testing that immediate and delayed return values work. Please review, thanks! This addresses bug HBASE-3899 . https://issues.apache.org/jira/browse/HBASE-3899 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java f2ae31e src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2f7dfae src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 60777cf Diff: https://reviews.apache.org/r/1257/diff Testing ------- Since the code is not used yet, it does not affect other areas of the codebase. Running unit tests as we speak. Thanks, Vlad
        Hide
        Todd Lipcon added a comment -

        +1, committing momentarily

        Show
        Todd Lipcon added a comment - +1, committing momentarily
        Hide
        Todd Lipcon added a comment -

        This is the patch I committed (r4 from reviewboard).

        Show
        Todd Lipcon added a comment - This is the patch I committed (r4 from reviewboard).
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2086 (See https://builds.apache.org/job/HBase-TRUNK/2086/)
        HBASE-3899. Add ability for delayed RPC calls to set return value immediately at call return.

        todd :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        • /hbase/trunk/CHANGES.txt
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2086 (See https://builds.apache.org/job/HBase-TRUNK/2086/ ) HBASE-3899 . Add ability for delayed RPC calls to set return value immediately at call return. todd : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Delayable.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java /hbase/trunk/CHANGES.txt
        Hide
        David S. Wang added a comment -

        Backport to 0.90.x branch.

        Show
        David S. Wang added a comment - Backport to 0.90.x branch.
        Hide
        David S. Wang added a comment -

        Would like to get this backported to 0.90.x branch, as HBASE-5973 depends on Delayable which is from this JIRA.

        Show
        David S. Wang added a comment - Would like to get this backported to 0.90.x branch, as HBASE-5973 depends on Delayable which is from this JIRA.
        Hide
        Hadoop QA added a comment -

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

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

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

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

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1844//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/12526481/HBASE-3899.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1844//console This message is automatically generated.
        Hide
        David S. Wang added a comment -

        On second thought, I might not need this to backport HBASE-5973.

        Show
        David S. Wang added a comment - On second thought, I might not need this to backport HBASE-5973 .

          People

          • Assignee:
            dhruba borthakur
            Reporter:
            dhruba borthakur
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development