Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.4.0
    • Component/s: java
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      A nonblocking RPC server based on Netty should be more scalable than the current implementation.

      We should provide two mechanisms for interfacing the RPC server to the implementations:

      1) "Blocking" RPC implementations run inside a worker threadpool. Implementators would not know that they're working in a non-blocking context.
      2) "Event-driven" RPC implementations that receive requests and some kind of request context. They are responsible for eventually calling context.respond(response) or somesuch. This would allow more scalable interaction with downstream services.

      I propose we focus on (1) first.

      1. ASF.LICENSE.NOT.GRANTED--AVRO-405.patch
        28 kB
        James Todd
      2. AVRO-405-coolwhy.patch
        23 kB
        harry wang
      3. AVRO-405-coolwhy-new.patch
        26 kB
        harry wang
      4. AVRO-405-for-review.patch
        10 kB
        Bo Shi
      5. netty-avro.zip
        18 kB
        James Todd

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          I think the server type 1 above could become the default Java RPC server implementation. It has the nice scalability properties of being able to handle a lot of concurrent connections, and as long as the RPC handler isn't doing further downstream RPCs, it shouldn't be much worse than #2.

          Show
          Todd Lipcon added a comment - I think the server type 1 above could become the default Java RPC server implementation. It has the nice scalability properties of being able to handle a lot of concurrent connections, and as long as the RPC handler isn't doing further downstream RPCs, it shouldn't be much worse than #2.
          Hide
          ryan rawson added a comment -

          I am interested in this, both needing it and working on it as well. Netty seems like a great framework for dealing with these.

          What protocol are you thinking about?

          Show
          ryan rawson added a comment - I am interested in this, both needing it and working on it as well. Netty seems like a great framework for dealing with these. What protocol are you thinking about?
          Hide
          Todd Lipcon added a comment -

          I'd like to do the protocol self-described as avro records. I think Matt Massie has some work on this somewhere - do we have an open JIRA for non-HTTP RPC?

          Also, SASL support is a strong requirement for this to be used in Hadoop.

          Show
          Todd Lipcon added a comment - I'd like to do the protocol self-described as avro records. I think Matt Massie has some work on this somewhere - do we have an open JIRA for non-HTTP RPC? Also, SASL support is a strong requirement for this to be used in Hadoop.
          Hide
          Philip Zeyliger added a comment -

          A Netty based server relates to figuring out the "server protocol" that we want to do.

          – Philip

          Show
          Philip Zeyliger added a comment - A Netty based server relates to figuring out the "server protocol" that we want to do. – Philip
          Hide
          Doug Cutting added a comment -

          Ning's posted an async Java HTTP client that might be useful for Avro.

          http://code.ning.com/2010/03/introducing-nings-asynchronous-http-client-library/

          Show
          Doug Cutting added a comment - Ning's posted an async Java HTTP client that might be useful for Avro. http://code.ning.com/2010/03/introducing-nings-asynchronous-http-client-library/
          Hide
          Scott Carey added a comment -

          Don't forget Apache HTTP Components:

          http://hc.apache.org/

          This is the evolution of the old Apache Commons HttpClient. HTTP Components adds server side components and asynchronous I/O capability.

          My experience with the old HttpClient was that it was significantly better performing and robust than the built-in sun java http client. Under high load with a lot of concurrent connections, there was a big (50% to 100%) difference.

          Show
          Scott Carey added a comment - Don't forget Apache HTTP Components: http://hc.apache.org/ This is the evolution of the old Apache Commons HttpClient. HTTP Components adds server side components and asynchronous I/O capability. My experience with the old HttpClient was that it was significantly better performing and robust than the built-in sun java http client. Under high load with a lot of concurrent connections, there was a big (50% to 100%) difference.
          Hide
          Bo Shi added a comment -

          Anyone working on this? I had a bit of free time this weekend to go through the avro and netty tutorials and threw together something that might fit the bill. The implementation is mostly plumbing; I put together frame encoding and decoding a handler that wraps a Responder instance. Would welcome any comments.

          Show
          Bo Shi added a comment - Anyone working on this? I had a bit of free time this weekend to go through the avro and netty tutorials and threw together something that might fit the bill. The implementation is mostly plumbing; I put together frame encoding and decoding a handler that wraps a Responder instance. Would welcome any comments.
          Hide
          Bo Shi added a comment -

          netty-based socket rpc implementation

          Show
          Bo Shi added a comment - netty-based socket rpc implementation
          Hide
          Todd Lipcon added a comment -

          Nice, Bo! I was planning on working on this but haven't had a chance to yet, so feel free to grab it. I'll try to take a look at your patch early this week.

          Show
          Todd Lipcon added a comment - Nice, Bo! I was planning on working on this but haven't had a chance to yet, so feel free to grab it. I'll try to take a look at your patch early this week.
          Hide
          Bo Shi added a comment -

          Thanks Todd; I am going to try to post an update within the next couple of days as (1) needs a bit of a re-org into multiple files instead of one monolithic source file and (b) the frame decoder needs some hardening against random/invalid/malicious input (e.g. AVRO-391 and THRIFT-601). Hopefully the async server will be less susceptible to memory-related issues.

          Show
          Bo Shi added a comment - Thanks Todd; I am going to try to post an update within the next couple of days as (1) needs a bit of a re-org into multiple files instead of one monolithic source file and (b) the frame decoder needs some hardening against random/invalid/malicious input (e.g. AVRO-391 and THRIFT-601 ). Hopefully the async server will be less susceptible to memory-related issues.
          Hide
          James Todd added a comment -

          hi bo -

          i have started piecing some some of this together and have placed the prototype code on github:

          http://github.com/jwtodd/netty-avro

          note: the project is sparse but does work to a degree

          i would love to collaborate on this work.

          • james
          Show
          James Todd added a comment - hi bo - i have started piecing some some of this together and have placed the prototype code on github: http://github.com/jwtodd/netty-avro note: the project is sparse but does work to a degree i would love to collaborate on this work. james
          Hide
          James Todd added a comment -

          attached is zip file that contains all of the netty/avro patch files as found in http://github.com/jwtodd/netty-avro minus the (large) libs/* files

          Show
          James Todd added a comment - attached is zip file that contains all of the netty/avro patch files as found in http://github.com/jwtodd/netty-avro minus the (large) libs/* files
          Hide
          James Todd added a comment -

          aim to merge patches and publish for review by this coming weekend

          Show
          James Todd added a comment - aim to merge patches and publish for review by this coming weekend
          Hide
          Bo Shi added a comment -

          Hi James,

          Some comments on a quick review of the current github link HEAD before you get too far into your merge

          These are mostly related to server-side, as I haven't been following async client implementation at all.

          • I'm warming up to chaining existing netty decoders as you have done in your pipeline definition although I think we should characterize the performance impact compared to implementing the decode/encode in one pass. Likely in real-world RPC implementations the decode/encode step becomes a very small part of the total computation performed per call, still though we can probably get some gains for cheap here (in particular LengthFieldBasedFrameDecoder [1] does way more than we need - maybe we just use a stripped down version - the implementation is very simple).
          • Taking the lead from existing client/server implementation in avro, our async server should implement the org.apache.avro.ipc.Server interface and the client should extend the org.apache.avro.ipc.Transceiver abstract class. Doing this, you won't have to modify the unit test in my patch and it will enable one liner switch to nonblocking rpc in projects like http://github.com/phunt/avro-rpc-quickstart
          • (minor) In the github repo, the package prototype.netty.avro is further divided into three sub-packages. It seems to me that a simple flat package structure is sufficient for this implementation (I don't forsee see the need to support more than one binary protocol).

          Todd L: Any word on when the streaming protocol will be finalized?

          Regards,
          Bo

          [1] http://grepcode.com/file/repository.jboss.com/maven2/org.jboss.netty/netty/3.1.0.BETA2/org/jboss/netty/handler/codec/frame/LengthFieldBasedFrameDecoder.java

          Show
          Bo Shi added a comment - Hi James, Some comments on a quick review of the current github link HEAD before you get too far into your merge These are mostly related to server-side, as I haven't been following async client implementation at all. I'm warming up to chaining existing netty decoders as you have done in your pipeline definition although I think we should characterize the performance impact compared to implementing the decode/encode in one pass. Likely in real-world RPC implementations the decode/encode step becomes a very small part of the total computation performed per call, still though we can probably get some gains for cheap here (in particular LengthFieldBasedFrameDecoder [1] does way more than we need - maybe we just use a stripped down version - the implementation is very simple). Taking the lead from existing client/server implementation in avro, our async server should implement the org.apache.avro.ipc.Server interface and the client should extend the org.apache.avro.ipc.Transceiver abstract class. Doing this, you won't have to modify the unit test in my patch and it will enable one liner switch to nonblocking rpc in projects like http://github.com/phunt/avro-rpc-quickstart (minor) In the github repo, the package prototype.netty.avro is further divided into three sub-packages. It seems to me that a simple flat package structure is sufficient for this implementation (I don't forsee see the need to support more than one binary protocol). Todd L: Any word on when the streaming protocol will be finalized? Regards, Bo [1] http://grepcode.com/file/repository.jboss.com/maven2/org.jboss.netty/netty/3.1.0.BETA2/org/jboss/netty/handler/codec/frame/LengthFieldBasedFrameDecoder.java
          Hide
          James Todd added a comment -

          great points bo.

          i took the approach of the protobuf codecs as the baseline and opted to not dig into the backing pipeline impl details. put another way, striving to "make it work" followed by "make it work right and make it work fast." definitely cut some corners. i would expect the netty impls to be efficient, though. on the other hand streamlining is not a bad thing.

          thx for the test heads up! that's definitely a good one to work in.

          yeah, likely over refactored the packages a bit. did that as a part of componentizing the impl given we are evaluating using this code inhouse. flattening things out is likely needed.

          i did leave a note on #netty for trustin to given him a heads up, see if the codec part of this work should/could reside w/ netty/etc.

          • james
          Show
          James Todd added a comment - great points bo. i took the approach of the protobuf codecs as the baseline and opted to not dig into the backing pipeline impl details. put another way, striving to "make it work" followed by "make it work right and make it work fast." definitely cut some corners. i would expect the netty impls to be efficient, though. on the other hand streamlining is not a bad thing. thx for the test heads up! that's definitely a good one to work in. yeah, likely over refactored the packages a bit. did that as a part of componentizing the impl given we are evaluating using this code inhouse. flattening things out is likely needed. i did leave a note on #netty for trustin to given him a heads up, see if the codec part of this work should/could reside w/ netty/etc. james
          Hide
          Todd Lipcon added a comment -

          Assigning to unassigned since I can't seem to assign to either of you two

          Regrading the streaming protocol, I don't think anyone is currently actively working on it – other things have eclipsed it on the great priority queue . If either of you wanted to submit a proposal that would be great!

          Show
          Todd Lipcon added a comment - Assigning to unassigned since I can't seem to assign to either of you two Regrading the streaming protocol, I don't think anyone is currently actively working on it – other things have eclipsed it on the great priority queue . If either of you wanted to submit a proposal that would be great!
          Hide
          James Todd added a comment -

          i'll grab this one as we work through the initial blocking netty patch.

          Show
          James Todd added a comment - i'll grab this one as we work through the initial blocking netty patch.
          Hide
          James Todd added a comment -

          deps:

          patch is relative to the [avro]/lang/java directory (ie cd [avro]/lang/java; patch -p0 < AVRO-405.patch)
          requires netty 3.1.5GA
          workaround: download netty-3.1.5.GA.jar and copy it to the build/lib dir (note: would be optimal to update ivy.xml)

          addressed:

          flattened package (see org.apache.avro.ipc.netty, opted to not co-populate o.a.a.ipc at this time)
          included ref/proto code (see org.apache.avro.ipc.netty.prototype)
          o.a.a.ipc.netty.NettyServer implements o.a.a.ipc.Server
          o.a.a.ipc.netty.NettyClient does not extend o.a.a.ipc.Transceiver
          note: .o.a.a.ipc.netty.AvroClientHandler includes an internal o.a.a.ipc.Transceiver for netty/avro delegation
          we should be able to reconstruct the tests accordingly
          opted to continue to use/leverage/delegate-to netty classes
          note: there should be upside in having the netty code look as netty-native as possible and optimizations should
          perhaps be considered downstream a bit, eg once we move past the current impl of accumulating ByteBuffer

          issues/todo:

          implement requestor/responder factory (eg via post handshake factory, etc)
          references to the Mail/Message prototype code (borrowed from avro-rpc-quickstart) persist until we can introspect
          the outbound/inbound message and make the relevant instantiations (see above issue)
          construct/support/migrate tests

          Show
          James Todd added a comment - deps: patch is relative to the [avro] /lang/java directory (ie cd [avro] /lang/java; patch -p0 < AVRO-405 .patch) requires netty 3.1.5GA workaround: download netty-3.1.5.GA.jar and copy it to the build/lib dir (note: would be optimal to update ivy.xml) addressed: flattened package (see org.apache.avro.ipc.netty, opted to not co-populate o.a.a.ipc at this time) included ref/proto code (see org.apache.avro.ipc.netty.prototype) o.a.a.ipc.netty.NettyServer implements o.a.a.ipc.Server o.a.a.ipc.netty.NettyClient does not extend o.a.a.ipc.Transceiver note: .o.a.a.ipc.netty.AvroClientHandler includes an internal o.a.a.ipc.Transceiver for netty/avro delegation we should be able to reconstruct the tests accordingly opted to continue to use/leverage/delegate-to netty classes note: there should be upside in having the netty code look as netty-native as possible and optimizations should perhaps be considered downstream a bit, eg once we move past the current impl of accumulating ByteBuffer issues/todo: implement requestor/responder factory (eg via post handshake factory, etc) references to the Mail/Message prototype code (borrowed from avro-rpc-quickstart) persist until we can introspect the outbound/inbound message and make the relevant instantiations (see above issue) construct/support/migrate tests
          Hide
          James Todd added a comment -

          hey bo shi, todd -

          attached is a first cut at merging our patches along with the associated notes (eg assumptions, addressed issues, todo issues, etc).

          i feel pretty good with this patch noting that there are a few open issues, namely we need to refactor out the explicit avro instance invocations
          for Mail/Message via request/responder post handshake factory delegation, something i can likely start to chew on should this patch look ok.

          hth,

          • james
          Show
          James Todd added a comment - hey bo shi, todd - attached is a first cut at merging our patches along with the associated notes (eg assumptions, addressed issues, todo issues, etc). i feel pretty good with this patch noting that there are a few open issues, namely we need to refactor out the explicit avro instance invocations for Mail/Message via request/responder post handshake factory delegation, something i can likely start to chew on should this patch look ok. hth, james
          Hide
          harry wang added a comment -

          I just submit my implementation (http://github.com/coolwhy/avro-rpc-on-netty) patch as another choice. Maybe we could make the final design better

          Show
          harry wang added a comment - I just submit my implementation ( http://github.com/coolwhy/avro-rpc-on-netty ) patch as another choice. Maybe we could make the final design better
          Hide
          harry wang added a comment -

          My implementation patch, anyone can try it

          Show
          harry wang added a comment - My implementation patch, anyone can try it
          Hide
          harry wang added a comment -

          oh, I found my patch is alike with Bo Shi's ... did I invent the wheel again ?

          Show
          harry wang added a comment - oh, I found my patch is alike with Bo Shi's ... did I invent the wheel again ?
          Hide
          harry wang added a comment -

          The patch file AVRO-405-coolwhy.patch I provided still has bugs. How can I delete it?

          Show
          harry wang added a comment - The patch file AVRO-405 -coolwhy.patch I provided still has bugs. How can I delete it?
          Hide
          Scott Carey added a comment -

          There is no reason to delete it. If you have an improved version, you can submit that one as well.

          We have had much delay in committing this feature. This is largely the fault of committers (like me) not reviewing the patches and providing feedback.

          I personally think this is a great contribution effort and that we'll want a Netty RPC implementation in 1.4.

          I won't have time to review this in the next couple weeks. I can't speak to the availability of any others to review.

          Show
          Scott Carey added a comment - There is no reason to delete it. If you have an improved version, you can submit that one as well. We have had much delay in committing this feature. This is largely the fault of committers (like me) not reviewing the patches and providing feedback. I personally think this is a great contribution effort and that we'll want a Netty RPC implementation in 1.4. I won't have time to review this in the next couple weeks. I can't speak to the availability of any others to review.
          Hide
          James Todd added a comment -

          hey scott -

          i welcome feedback re AVRO-405.patch, please see above comments for associated context.

          remaining work, assuming the work thus far is viable/etc (i have received feedback from several indicating it works as expected), includes:

          (resolve TODO) be adding a responder delegate interface that is invoked during request handshake but before request processing

          use latest netty distribution (should be trivial)

          build alternative protocol specified by bruce

          i would appreciate a sync check/feedback for the current patch before proceeding. again, there should be ample review context in the above associated comments and the patch proper.

          hth,

          • james
          Show
          James Todd added a comment - hey scott - i welcome feedback re AVRO-405 .patch, please see above comments for associated context. remaining work, assuming the work thus far is viable/etc (i have received feedback from several indicating it works as expected), includes: (resolve TODO) be adding a responder delegate interface that is invoked during request handshake but before request processing use latest netty distribution (should be trivial) build alternative protocol specified by bruce i would appreciate a sync check/feedback for the current patch before proceeding. again, there should be ample review context in the above associated comments and the patch proper. hth, james
          Hide
          Doug Cutting added a comment -

          I hope to have time later this week to review this. At a glance, it looked like it could be nearly ready to commit.

          > adding a responder delegate interface that is invoked during request handshake but before request processing

          That sounds like a separate issue. Why is it required before we commit Harry's patch?

          > build alternative protocol specified by bruce

          That's also a separate issue. Having a Netty-based Transceiver and Server would be useful and Harry's patch appears to provide a complete implementation. Long-term we should consider improving responder delegation, and we need to specify an RPC transport that's high-performance than HTTP. But those shouldn't prevent the addition of a simple Netty-based Transceiver and Server now, should they?

          Show
          Doug Cutting added a comment - I hope to have time later this week to review this. At a glance, it looked like it could be nearly ready to commit. > adding a responder delegate interface that is invoked during request handshake but before request processing That sounds like a separate issue. Why is it required before we commit Harry's patch? > build alternative protocol specified by bruce That's also a separate issue. Having a Netty-based Transceiver and Server would be useful and Harry's patch appears to provide a complete implementation. Long-term we should consider improving responder delegation, and we need to specify an RPC transport that's high-performance than HTTP. But those shouldn't prevent the addition of a simple Netty-based Transceiver and Server now, should they?
          Hide
          harry wang added a comment -

          My new patch to make the NettyTransceiver look better. The transport protocol has been changed to support some async call feature in the client side.

          Show
          harry wang added a comment - My new patch to make the NettyTransceiver look better. The transport protocol has been changed to support some async call feature in the client side.
          Hide
          Doug Cutting added a comment -

          I just committed this. Thanks, Harry!

          Show
          Doug Cutting added a comment - I just committed this. Thanks, Harry!

            People

            • Assignee:
              harry wang
              Reporter:
              Todd Lipcon
            • Votes:
              1 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development