Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.4.1
    • Fix Version/s: None
    • Component/s: c
    • Labels:
      None

      Description

      Provide support for RPC with Avro-C

      1. AVRO-777-rpc-c.zip
        20 kB
        Gilles Gaillard
      2. sample_idl_and_generated.zip
        11 kB
        Gilles Gaillard

        Activity

        Hide
        Gilles Gaillard added a comment -

        The attached file contains a patch and implements partial support for RPC with Avro-C based on r1063596 and relies on the patch previously submitted in AVRO-767.

        It enables generation of C code based on the avdl definition and Velocity templates. This is implemented using ant tasks:

        • an 'idl' task to generate code - see the provided classes IdlTask.java and CHelper.java
        • common-ipc-avro.xml is an example of how to use this ant task together with the ProtocolTask and SchemaTask
        • velocity templates:
        • c_macro.vm, c_protocol_body.vm, c_protocol_header.vm: support for C code generation
        • java_enum.vm: support for the 'partial' RPC (see below)

        RPC support is partial, as the AVRO protocol is only partially implemented:

        • handshake is not implemented, assuming that both sides have the same version.
        • metadata not included in the message
        • message discriminant is an int instead of the name of the message
          (this explains why we generate a java enum for the messages).
        • only oneway messages are supported

        The patch for the C code contains a generic reader and writer.
        All basics types are supported except fixed, array, map, float and double.

        The rpc part is managed in rpc.c.

        Show
        Gilles Gaillard added a comment - The attached file contains a patch and implements partial support for RPC with Avro-C based on r1063596 and relies on the patch previously submitted in AVRO-767 . It enables generation of C code based on the avdl definition and Velocity templates. This is implemented using ant tasks: an 'idl' task to generate code - see the provided classes IdlTask.java and CHelper.java common-ipc-avro.xml is an example of how to use this ant task together with the ProtocolTask and SchemaTask velocity templates: c_macro.vm, c_protocol_body.vm, c_protocol_header.vm: support for C code generation java_enum.vm: support for the 'partial' RPC (see below) RPC support is partial, as the AVRO protocol is only partially implemented: handshake is not implemented, assuming that both sides have the same version. metadata not included in the message message discriminant is an int instead of the name of the message (this explains why we generate a java enum for the messages). only oneway messages are supported The patch for the C code contains a generic reader and writer. All basics types are supported except fixed, array, map, float and double. The rpc part is managed in rpc.c.
        Hide
        Doug Cutting added a comment -

        This is great to see! Can you please convert it to a single patch file? That will make it easier for folks to apply and review it.

        Glancing at the code, I'm not sure why you need to generate a Java enum of the message names for each protocol (java_enum.vm). I can't see where that's used.

        DCreager: will you have chance to review this more closely?

        Show
        Doug Cutting added a comment - This is great to see! Can you please convert it to a single patch file? That will make it easier for folks to apply and review it. Glancing at the code, I'm not sure why you need to generate a Java enum of the message names for each protocol (java_enum.vm). I can't see where that's used. DCreager: will you have chance to review this more closely?
        Hide
        Douglas Creager added a comment -

        I've looked through this a bit, but I'll need a single patch file before I can start testing things. Also, there's not much in the way of comments; can you walk us through how all of the pieces fit together?

        A couple of things that stand out:

        • Can you verify that this patch works on the latest 1.5.0 release? We made some changes to the avro_datum_t API (AVRO-751), but it looks like you're using the old 1.4.x API.
        • You include memory.h, but I think that with the modern C standards that header is deprecated in favor of string.h. string.h gives you all of the mem* functions that you're using.
        • You also have to be careful about which Avro headers you're using in your generated code. The only headers that are installed are avro.h, and the header files in src/avro. The others are only available while compiling the Avro library itself.
        • What platform are you testing on? I think your semaphore implementation (sem_t) won't work on Mac OS X. But that's nicely segregated, so hopefully we can drop something else in pretty easily.
        • An aesthetic point — I'm not sure how picky we're being about formatting guidelines, but the rest of the Avro C library uses the Linux formatting conventions: indent with actual tab characters, tab width is 8 spaces. (It's not my favorite, but I've been aiming for consistency when writing my own patches...)
        • The fact that you're using pthreads might make this not work on Windows machines. Bruce has more experience on the Win32 side of things, so he can hopefully fill us in on that.

        I hope that list doesn't seem too pessimistic — I'm glad to see this contribution, too!

        My last concern is with the code generation. Can you describe what's being produced by the velocity templates? Can you walk through which parts are generated code, and which parts are provided by the Avro library itself?

        I have a set of patches I've been working on for generating custom C types for Avro schemas (AVRO-778), so I'd like to make sure that there isn't too much overlap. My schema compiler doesn't use Velocity, though; it's written in C. I did this b/c I don't want to require users of the C library to install the Java library as well, just to be able to do code generation.

        Show
        Douglas Creager added a comment - I've looked through this a bit, but I'll need a single patch file before I can start testing things. Also, there's not much in the way of comments; can you walk us through how all of the pieces fit together? A couple of things that stand out: Can you verify that this patch works on the latest 1.5.0 release? We made some changes to the avro_datum_t API ( AVRO-751 ), but it looks like you're using the old 1.4.x API. You include memory.h, but I think that with the modern C standards that header is deprecated in favor of string.h. string.h gives you all of the mem* functions that you're using. You also have to be careful about which Avro headers you're using in your generated code. The only headers that are installed are avro.h, and the header files in src/avro. The others are only available while compiling the Avro library itself. What platform are you testing on? I think your semaphore implementation (sem_t) won't work on Mac OS X. But that's nicely segregated, so hopefully we can drop something else in pretty easily. An aesthetic point — I'm not sure how picky we're being about formatting guidelines, but the rest of the Avro C library uses the Linux formatting conventions: indent with actual tab characters, tab width is 8 spaces. (It's not my favorite, but I've been aiming for consistency when writing my own patches...) The fact that you're using pthreads might make this not work on Windows machines. Bruce has more experience on the Win32 side of things, so he can hopefully fill us in on that. I hope that list doesn't seem too pessimistic — I'm glad to see this contribution, too! My last concern is with the code generation. Can you describe what's being produced by the velocity templates? Can you walk through which parts are generated code, and which parts are provided by the Avro library itself? I have a set of patches I've been working on for generating custom C types for Avro schemas ( AVRO-778 ), so I'd like to make sure that there isn't too much overlap. My schema compiler doesn't use Velocity, though; it's written in C. I did this b/c I don't want to require users of the C library to install the Java library as well, just to be able to do code generation.
        Hide
        Gilles Gaillard added a comment -

        We're running a bit out of time currently so we'll come back hopefully soon with more details and yes we'll try to move to 1.5.0 API (side note: would be nice if next release be announced to 'users' sometimes before the final vote). Here are some quick answers though:

        • what we provided represents what we needed at the time of writing not the ideal
        • for a quick look I attached a sample of idl file and the corresponding generated files. Note that the use of templates enables easy customization, e.g. prefix for method names or custom includes (there is one in the templates for pointing to a custom error type).
        • we have no problem in using java but understand others might This was just the quickest way to go for us for code generation.
        • the java_enum is here only because we decided in a first implementation to use for message dispatching an int instead of the message name (so we have a custom java implementation). Obviously this must be ignored for real-future use.
        • we are testing on Linux so beside threads and memory may need also some change for sockets.
        Show
        Gilles Gaillard added a comment - We're running a bit out of time currently so we'll come back hopefully soon with more details and yes we'll try to move to 1.5.0 API (side note: would be nice if next release be announced to 'users' sometimes before the final vote). Here are some quick answers though: what we provided represents what we needed at the time of writing not the ideal for a quick look I attached a sample of idl file and the corresponding generated files. Note that the use of templates enables easy customization, e.g. prefix for method names or custom includes (there is one in the templates for pointing to a custom error type). we have no problem in using java but understand others might This was just the quickest way to go for us for code generation. the java_enum is here only because we decided in a first implementation to use for message dispatching an int instead of the message name (so we have a custom java implementation). Obviously this must be ignored for real-future use. we are testing on Linux so beside threads and memory may need also some change for sockets.

          People

          • Assignee:
            Unassigned
            Reporter:
            Gilles Gaillard
          • Votes:
            5 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:

              Development