Avro
  1. Avro
  2. AVRO-321

Run Java RPC interop tests again

    Details

    • Type: Test Test
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0
    • Component/s: build, java
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    1. AVRO-321.patch
      19 kB
      Doug Cutting
    2. AVRO-321.patch
      18 kB
      Doug Cutting
    3. AVRO-321.patch
      3 kB
      Doug Cutting

      Issue Links

        Activity

        Hide
        Jeff Hammerbacher added a comment -

        Question: should interop tests use raw sockets or HTTP as a transport, or both?

        Show
        Jeff Hammerbacher added a comment - Question: should interop tests use raw sockets or HTTP as a transport, or both?
        Hide
        Doug Cutting added a comment -

        Perhaps we should use command line tools for RPC interop testing?

        Each language could have send and recieve commands with identical parameters, e.g.:
        send transport host port protocol messageName requestData
        receive transport port protocol messageName responseData

        Then we could have a top-level testing script that invokes all of these against one another.

        This would either require that we find a way to encode binary request and response data on the command line, or that all implementations support the JSON encoding.

        Thoughts?

        Show
        Doug Cutting added a comment - Perhaps we should use command line tools for RPC interop testing? Each language could have send and recieve commands with identical parameters, e.g.: send transport host port protocol messageName requestData receive transport port protocol messageName responseData Then we could have a top-level testing script that invokes all of these against one another. This would either require that we find a way to encode binary request and response data on the command line, or that all implementations support the JSON encoding. Thoughts?
        Hide
        Jeff Hammerbacher added a comment -

        To clarify: I used "raw sockets" inappropriately, as pointed out by Chad Harrington on the maling list. I just meant TCP/IP sockets?

        Show
        Jeff Hammerbacher added a comment - To clarify: I used "raw sockets" inappropriately, as pointed out by Chad Harrington on the maling list. I just meant TCP/IP sockets?
        Hide
        Philip Zeyliger added a comment -

        Command line tools would be an easy way to standardize, yes.

        Show
        Philip Zeyliger added a comment - Command line tools would be an easy way to standardize, yes.
        Hide
        Doug Cutting added a comment -

        The problem with the existing Java command line RPC tools is that they presuppose a json decoder implementation.

        To amend this, I propose that, if request or response data is not specified on the command line, then the command line tool should generate a random datum and use that. Similarly, if the message name is not specified, then the client tool should send a random message from the protocol and the server tool should respond to any message in the protocol.

        The remaining problem is how the server should communicate its port. I propose that, if the port passed to a server is zero, indicating that a port should be allocated dynamically, then the server should write the port to standard output as its first and only line. Then a client shell script can do something like:

        avro-java-tool rpcreceive "http://0.0.0.0:0/" "$protocol" > port &
        sleep 1 # wait for server to start & write port
        avro-py-tool rpcsend "http://0.0.0.0:`cat port`" "$protocol"
        wait # wait for server to exit
        

        This would generate a random call from Python to Java. The top-level interop script would do this some number of times for each language pair.

        Show
        Doug Cutting added a comment - The problem with the existing Java command line RPC tools is that they presuppose a json decoder implementation. To amend this, I propose that, if request or response data is not specified on the command line, then the command line tool should generate a random datum and use that. Similarly, if the message name is not specified, then the client tool should send a random message from the protocol and the server tool should respond to any message in the protocol. The remaining problem is how the server should communicate its port. I propose that, if the port passed to a server is zero, indicating that a port should be allocated dynamically, then the server should write the port to standard output as its first and only line. Then a client shell script can do something like: avro-java-tool rpcreceive "http: //0.0.0.0:0/" "$protocol" > port & sleep 1 # wait for server to start & write port avro-py-tool rpcsend "http: //0.0.0.0:`cat port`" "$protocol" wait # wait for server to exit This would generate a random call from Python to Java. The top-level interop script would do this some number of times for each language pair.
        Hide
        Matt Massie added a comment -

        +1 on the plan

        We could also avoid the sleep by making the script look more like

        #!/bin/sh
        avro-java-tool rpcreceive "http://0.0.0.0:0/" "$protocol" | read port
        avro-py-tool rpcsend "http://0.0.0.0:$port" "$protocol"
        
        Show
        Matt Massie added a comment - +1 on the plan We could also avoid the sleep by making the script look more like #!/bin/sh avro-java-tool rpcreceive "http: //0.0.0.0:0/" "$protocol" | read port avro-py-tool rpcsend "http: //0.0.0.0:$port" "$protocol"
        Hide
        Philip Zeyliger added a comment -

        I don't really like missing arguments to rpcsend triggering arbitrary things to be sent. I'd like to use rpcsend as an interactive tool to interact with real servers, and adding non-determinism seems not nice for that. At this point, there's enough special behavior; could we just name the tool "interop", and use that? It may share code or behavior with "rpcsend", but I think it would be clearer if it the name were independent.

        Alternately, I think it's totally fair game to generate or check-in sample data (perhaps in AvroDataFile format) to the repo and have a command-line flag that reads the input from a file. That avoids the JSON decoder problem, since the file has binary encoded data.

        Show
        Philip Zeyliger added a comment - I don't really like missing arguments to rpcsend triggering arbitrary things to be sent. I'd like to use rpcsend as an interactive tool to interact with real servers, and adding non-determinism seems not nice for that. At this point, there's enough special behavior; could we just name the tool "interop", and use that? It may share code or behavior with "rpcsend", but I think it would be clearer if it the name were independent. Alternately, I think it's totally fair game to generate or check-in sample data (perhaps in AvroDataFile format) to the repo and have a command-line flag that reads the input from a file. That avoids the JSON decoder problem, since the file has binary encoded data.
        Hide
        Doug Cutting added a comment -

        Here's a patch that contains a script to generate RPC interop data. It writes files in share/test/interop/rpc/$message/$call/

        {request,response}

        .avro, where $message is the message name and $call is a directory per request/response pair. The request and response data are in avro data files that contain a single request and response. Currently the script generates calls for the messages in simple.avpr.

        My idea is that we commit the output of this script, then use command line tools in each language to read these and send the request data and check that the response data matches, etc.

        Does this look like the right direction for rpc interop?

        Show
        Doug Cutting added a comment - Here's a patch that contains a script to generate RPC interop data. It writes files in share/test/interop/rpc/$message/$call/ {request,response} .avro, where $message is the message name and $call is a directory per request/response pair. The request and response data are in avro data files that contain a single request and response. Currently the script generates calls for the messages in simple.avpr. My idea is that we commit the output of this script, then use command line tools in each language to read these and send the request data and check that the response data matches, etc. Does this look like the right direction for rpc interop?
        Hide
        Philip Zeyliger added a comment -

        +1.

        I agree that we should commit the outputs (as well as the script that generated them).

        Show
        Philip Zeyliger added a comment - +1. I agree that we should commit the outputs (as well as the script that generated them).
        Hide
        Doug Cutting added a comment -

        Here's a complete version of RPC interop tests. It currently only runs java-versus-java, but is intended to handle all language pairs.

        It assumes that each implementation will have a send and receive script that accept the following parameters:

        uri protocol message -file data

        'uri' is an HTTP url naming the service. The server script is started with URI of http://0.0.0.0:0/, meaning to start a server on a newly allocated port on every interface. The server script should print to standard output 'Port: $PORT' to identify the port used.

        'protocol' names a file containing an Avro protocol to be used.

        'message' names a message within that protocol.

        'data' names an Avro data file with a single datum. The datum passed to the server is a record matching the message's parameter schema. The datum passed to the client matches the message's return type.

        The server script should accept one request and exit. The client script should send one request and exit.

        Show
        Doug Cutting added a comment - Here's a complete version of RPC interop tests. It currently only runs java-versus-java, but is intended to handle all language pairs. It assumes that each implementation will have a send and receive script that accept the following parameters: uri protocol message -file data 'uri' is an HTTP url naming the service. The server script is started with URI of http://0.0.0.0:0/ , meaning to start a server on a newly allocated port on every interface. The server script should print to standard output 'Port: $PORT' to identify the port used. 'protocol' names a file containing an Avro protocol to be used. 'message' names a message within that protocol. 'data' names an Avro data file with a single datum. The datum passed to the server is a record matching the message's parameter schema. The datum passed to the client matches the message's return type. The server script should accept one request and exit. The client script should send one request and exit.
        Hide
        Philip Zeyliger added a comment -

        Looks good overall. The proof will be when we add a language or two to this suite. Also, should there be an ant target that runs at least what we have here? Or, rather, a build.sh target?

        For my edification/education, what's the difference between '@' and '$' in ant, in the segment below?

                <sysproperty key="test.dir" value="@{test.dir}"/>
                <sysproperty key="share.dir" value="${share.dir}"/>
        

        RpcReceiveTool.java--sleep(1000)

        Why did you need to insert a sleep in here?

        Usage: uri protocolFile message_name [ -data d | -file f ]"

        I think this might be more appropriately "uri protocol_file message_name (-data d|-file f)" to indicate that one of data or file is required. We should file a bug for the fact that HttpServer doesn't let you specify what IP to bind to. (Same goes for RpcSendTool.)

        We should in theory check the scheme of the URI too for valid values.

        datum = Util.datumFromFile(file.value(opts));

        Do we need to check that the file's schema and the protocol schema are identical? Or, rather, read the file using the protocol schema: yes, I think we need to do the latter.

        static Object datumFromFile(String file)

        I'm a bit of a worrywart: if this sends only one request (it's conceivable for it to replay a set of them), we should perhaps check that in.hasNext() is false.

        TestRpcReceiveAndSendTools.java

        There's no test for the "-file" path.

        share/test/interop/bin/test_rpc_interop.sh: sleep 1

        Might be nice to leave a comment why that sleep needs to be there. I tried a couple of things, but couldn't get around it.

        Show
        Philip Zeyliger added a comment - Looks good overall. The proof will be when we add a language or two to this suite. Also, should there be an ant target that runs at least what we have here? Or, rather, a build.sh target? For my edification/education, what's the difference between '@' and '$' in ant, in the segment below? <sysproperty key="test.dir" value="@{test.dir}"/> <sysproperty key="share.dir" value="${share.dir}"/> RpcReceiveTool.java--sleep(1000) Why did you need to insert a sleep in here? Usage: uri protocolFile message_name [ -data d | -file f ]" I think this might be more appropriately "uri protocol_file message_name (- data d| -file f)" to indicate that one of data or file is required. We should file a bug for the fact that HttpServer doesn't let you specify what IP to bind to. (Same goes for RpcSendTool.) We should in theory check the scheme of the URI too for valid values. datum = Util.datumFromFile(file.value(opts)); Do we need to check that the file's schema and the protocol schema are identical? Or, rather, read the file using the protocol schema: yes, I think we need to do the latter. static Object datumFromFile(String file) I'm a bit of a worrywart: if this sends only one request (it's conceivable for it to replay a set of them), we should perhaps check that in.hasNext() is false. TestRpcReceiveAndSendTools.java There's no test for the "-file" path. share/test/interop/bin/test_rpc_interop.sh: sleep 1 Might be nice to leave a comment why that sleep needs to be there. I tried a couple of things, but couldn't get around it.
        Hide
        Doug Cutting added a comment -

        > Or, rather, a build.sh target?

        Oops. I meant to add it to the 'test' target there. Done.

        > what's the difference between '@' and '$' in ant, in the segment below

        @ refers to a task parameter, while $ refers to a property. They're different namespaces in Ant.

        > RpcReceiveTool.java--sleep(1000)

        Without that the server would sometimes exit before it had written the response back to the client. There's probably a better way of doing this. Ideas?

        > I think this might be more appropriately "uri protocol_file message_name (-data d|-file f)" to indicate that one of data or file is required.

        Done.

        > read the file using the protocol schema:

        Done.

        > we should perhaps check that in.hasNext() is false.

        I don't see the point. This method is defined to read and return the first entry from a file, not the only entry.

        > There's no test for the "-file" path.

        That's now tested by the top-level build.

        > Might be nice to leave a comment why that sleep needs to be there.

        Done.

        Show
        Doug Cutting added a comment - > Or, rather, a build.sh target? Oops. I meant to add it to the 'test' target there. Done. > what's the difference between '@' and '$' in ant, in the segment below @ refers to a task parameter, while $ refers to a property. They're different namespaces in Ant. > RpcReceiveTool.java--sleep(1000) Without that the server would sometimes exit before it had written the response back to the client. There's probably a better way of doing this. Ideas? > I think this might be more appropriately "uri protocol_file message_name (-data d|-file f)" to indicate that one of data or file is required. Done. > read the file using the protocol schema: Done. > we should perhaps check that in.hasNext() is false. I don't see the point. This method is defined to read and return the first entry from a file, not the only entry. > There's no test for the "-file" path. That's now tested by the top-level build. > Might be nice to leave a comment why that sleep needs to be there. Done.
        Hide
        Doug Cutting added a comment -

        This patch subsumes AVRO-324, which should be closed if this patch is committed.

        Show
        Doug Cutting added a comment - This patch subsumes AVRO-324 , which should be closed if this patch is committed.
        Hide
        Philip Zeyliger added a comment -

        > RpcReceiveTool.java--sleep(1000)

        Without that the server would sometimes exit before it had written the response back to the client. There's probably a better way of doing this. Ideas?

        Perhaps change HttpServer to use Server.setGracefulShutdown(2000 /* 2 seconds); in stop()? See http://jetty.codehaus.org/jetty/jetty-6/apidocs/index.html?org/mortbay/jetty/Server.html

        If that works (and even if it doesn't), go ahead and commit! +1.

        Show
        Philip Zeyliger added a comment - > RpcReceiveTool.java--sleep(1000) Without that the server would sometimes exit before it had written the response back to the client. There's probably a better way of doing this. Ideas? Perhaps change HttpServer to use Server.setGracefulShutdown(2000 /* 2 seconds); in stop()? See http://jetty.codehaus.org/jetty/jetty-6/apidocs/index.html?org/mortbay/jetty/Server.html If that works (and even if it doesn't), go ahead and commit! +1.
        Hide
        Jeff Hammerbacher added a comment -

        Hey Doug: could you add a "description" attribute to the target named "interop-data-generate" so that it shows up when running ant -p?

        Show
        Jeff Hammerbacher added a comment - Hey Doug: could you add a "description" attribute to the target named "interop-data-generate" so that it shows up when running ant -p ?
        Hide
        Jeff Hammerbacher added a comment -

        I have data file interop working for Python over in AVRO-399. Question: should we test different codecs in interop, and should we do more than just assert non-null? That is, should we try to validate values?

        Also, I did not implement interop-data-generate--I didn't see the point of having that implemented in more than one language, particularly if we check in the raw data itself.

        Show
        Jeff Hammerbacher added a comment - I have data file interop working for Python over in AVRO-399 . Question: should we test different codecs in interop, and should we do more than just assert non-null? That is, should we try to validate values? Also, I did not implement interop-data-generate--I didn't see the point of having that implemented in more than one language, particularly if we check in the raw data itself.
        Hide
        Jeff Hammerbacher added a comment -

        Can you also set the svn properties on the share/test/interop/bin/test_rpc_interop.sh to be executable?

        Show
        Jeff Hammerbacher added a comment - Can you also set the svn properties on the share/test/interop/bin/test_rpc_interop.sh to be executable?
        Hide
        Jeff Hammerbacher added a comment -

        Do the same for {[lang/java/src/test/bin/gen_rpc_interop.sh}} and add an ant task for it?

        Show
        Jeff Hammerbacher added a comment - Do the same for {[lang/java/src/test/bin/gen_rpc_interop.sh}} and add an ant task for it?
        Hide
        Jeff Hammerbacher added a comment -

        Perhaps change HttpServer to use Server.setGracefulShutdown(2000 /* 2 seconds); in stop()? See http://jetty.codehaus.org/jetty/jetty-6/apidocs/index.html?org/mortbay/jetty/Server.html

        I'm hitting this in https://issues.apache.org/jira/browse/AVRO-287

        Show
        Jeff Hammerbacher added a comment - Perhaps change HttpServer to use Server.setGracefulShutdown(2000 /* 2 seconds); in stop()? See http://jetty.codehaus.org/jetty/jetty-6/apidocs/index.html?org/mortbay/jetty/Server.html I'm hitting this in https://issues.apache.org/jira/browse/AVRO-287
        Hide
        Doug Cutting added a comment -

        I just committed this.

        > Perhaps change HttpServer to use Server.setGracefulShutdown(2000 /* 2 seconds); in stop()?

        I'd rather not make stop() always delay. We only need the delay here, I think, when we're stopping from within a response.

        > could you add a "description" attribute to the target named "interop-data-generate" so that it shows up when running ant -p?

        Done.

        > should we test different codecs in interop, and should we do more than just assert non-null? That is, should we try to validate values?

        Good stuff for later. Let's walk before we run. We should test codecs. We should validate.

        > I did not implement interop-data-generate--I didn't see the point of having that implemented in more than one language, particularly if we check in the raw data itself.

        I don't follow. How do we otherwise know that a Java program can read a file written by Python?

        > Can you also set the svn properties on the share/test/interop/bin/test_rpc_interop.sh to be executable?

        That was already done. The problem is that the patch program ignores this.

        Show
        Doug Cutting added a comment - I just committed this. > Perhaps change HttpServer to use Server.setGracefulShutdown(2000 /* 2 seconds); in stop()? I'd rather not make stop() always delay. We only need the delay here, I think, when we're stopping from within a response. > could you add a "description" attribute to the target named "interop-data-generate" so that it shows up when running ant -p? Done. > should we test different codecs in interop, and should we do more than just assert non-null? That is, should we try to validate values? Good stuff for later. Let's walk before we run. We should test codecs. We should validate. > I did not implement interop-data-generate--I didn't see the point of having that implemented in more than one language, particularly if we check in the raw data itself. I don't follow. How do we otherwise know that a Java program can read a file written by Python? > Can you also set the svn properties on the share/test/interop/bin/test_rpc_interop.sh to be executable? That was already done. The problem is that the patch program ignores this.
        Hide
        Jeff Hammerbacher added a comment -

        I don't follow. How do we otherwise know that a Java program can read a file written by Python?

        Umm, you're right. That's what I get for working on code after 4 am. Will add a Python data generator to AVRO-399.

        Show
        Jeff Hammerbacher added a comment - I don't follow. How do we otherwise know that a Java program can read a file written by Python? Umm, you're right. That's what I get for working on code after 4 am. Will add a Python data generator to AVRO-399 .
        Hide
        Philip Zeyliger added a comment -
        > Perhaps change HttpServer to use Server.setGracefulShutdown(2000 /* 2 seconds); in stop()?
        
        I'd rather not make stop() always delay. We only need the delay here, I think, when we're stopping from within a response.
        

        I don't think setGracefulShutdown always delays. It delays until existing sockets have been closed, or something like that. Or at least that's my interpretation of their API.

        – Philip

        Show
        Philip Zeyliger added a comment - > Perhaps change HttpServer to use Server.setGracefulShutdown(2000 /* 2 seconds); in stop()? I'd rather not make stop() always delay. We only need the delay here, I think, when we're stopping from within a response. I don't think setGracefulShutdown always delays. It delays until existing sockets have been closed, or something like that. Or at least that's my interpretation of their API. – Philip
        Hide
        Doug Cutting added a comment -

        Reading the Jetty documentation, it looks like the graceful shutdown period is the amount of time it will wait for requests in progress to complete. I tried specifying this however, and it caused unit tests to run considerably slower, not a good thing.

        Show
        Doug Cutting added a comment - Reading the Jetty documentation, it looks like the graceful shutdown period is the amount of time it will wait for requests in progress to complete. I tried specifying this however, and it caused unit tests to run considerably slower, not a good thing.

          People

          • Assignee:
            Doug Cutting
            Reporter:
            Jeff Hammerbacher
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development