Apache Jena
  1. Apache Jena
  2. JENA-240

If the remote SPARQL Service endpoint does not respond the query hangs, Adding timeout to the Service should fix the problem

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: ARQ 2.9.0, ARQ 2.9.1
    • Fix Version/s: ARQ 2.9.1
    • Component/s: ARQ
    • Labels:
      None

      Description

      When creating the HttpQuery object the com.hp.hpl.jena.sparql.engine.http.Service class does not set any timeout values. HttpQuery will hang if the remote endpoint does not respond.

      I think the solution is to allow the context to specify the timeout values (and perhaps other setter values on HttpQuery). The context should have default values and provide a mechanism to override the defaults based on the URL. Similar to the way query parameters are configured after JENA-195.

      1. patch.txt
        10 kB
        Claude Warren
      2. patch.txt
        7 kB
        Claude Warren
      3. TestService.java
        3 kB
        Claude Warren
      4. TestService.java
        3 kB
        Claude Warren
      5. ServiceDocumentation.odt
        14 kB
        Claude Warren

        Activity

        Hide
        Claude Warren added a comment -

        Patch. All changes in com.hp.hpl.jena.sparql.engine.http.Service so patch is rooted in that file.

        Show
        Claude Warren added a comment - Patch. All changes in com.hp.hpl.jena.sparql.engine.http.Service so patch is rooted in that file.
        Hide
        Rob Vesse added a comment -

        Hi Claude, thanks for your contribution, I just have a couple of questions for you:

        How was this patch generated?

        It appears to replace almost the entire class some bits of which appear to have minimal changes which suggests a mismatch in your SVN EOL settings, this is an issue I've run into in the past when developing patches on Windows - see http://mail-archives.apache.org/mod_mbox/incubator-jena-dev/201111.mbox/%3CC0B6979A3CA668458B697E4EA907CA9401825F@CFWEX01.americas.cray.com%3E for ways to solve this. If you are able to regenerate the patch so that it contains only the actual changes that would be preferred as it is much easier to review and integrate if the patch only contains your changes.

        Also have you considered writing any tests for this?

        It should be fairly trivial to write a test to a known non-existent server (maybe localhost on some random port) which demonstrates that timeouts do apply as expected after these changes. While lack of tests is not a barrier to integration it's another things that makes it easier to review and integrate

        Show
        Rob Vesse added a comment - Hi Claude, thanks for your contribution, I just have a couple of questions for you: How was this patch generated? It appears to replace almost the entire class some bits of which appear to have minimal changes which suggests a mismatch in your SVN EOL settings, this is an issue I've run into in the past when developing patches on Windows - see http://mail-archives.apache.org/mod_mbox/incubator-jena-dev/201111.mbox/%3CC0B6979A3CA668458B697E4EA907CA9401825F@CFWEX01.americas.cray.com%3E for ways to solve this. If you are able to regenerate the patch so that it contains only the actual changes that would be preferred as it is much easier to review and integrate if the patch only contains your changes. Also have you considered writing any tests for this? It should be fairly trivial to write a test to a known non-existent server (maybe localhost on some random port) which demonstrates that timeouts do apply as expected after these changes. While lack of tests is not a barrier to integration it's another things that makes it easier to review and integrate
        Hide
        Claude Warren added a comment -

        I developed the patch on a Ubuntu box using Eclipse. I'll see if I can regenerate it as you noted. Though I have to admit that most of the source file has changed.

        I like your test idea and will try to get to that tomorrow.

        My main concern with the code are the Symbols that I created in the Service class. Perhaps they should be in ARQ but then I am not certain that the names they have now would be appropraite. Thought on this would be appreciated.

        Show
        Claude Warren added a comment - I developed the patch on a Ubuntu box using Eclipse. I'll see if I can regenerate it as you noted. Though I have to admit that most of the source file has changed. I like your test idea and will try to get to that tomorrow. My main concern with the code are the Symbols that I created in the Service class. Perhaps they should be in ARQ but then I am not certain that the names they have now would be appropraite. Thought on this would be appreciated.
        Hide
        Paolo Castagna added a comment -

        Claude, first of all, thanks for opening the issue and for your patch.
        You can use svn diff command to create a patch if you want, see here:
        http://incubator.apache.org/jena/getting_involved/index.html#submit-your-patches
        In general, I prefer patches to be "rooted" to the trunk rather than a single file or a directory (much easier, no need to communicate how/where to apply the patch, works well with changes in multiple files).

        Show
        Paolo Castagna added a comment - Claude, first of all, thanks for opening the issue and for your patch. You can use svn diff command to create a patch if you want, see here: http://incubator.apache.org/jena/getting_involved/index.html#submit-your-patches In general, I prefer patches to be "rooted" to the trunk rather than a single file or a directory (much easier, no need to communicate how/where to apply the patch, works well with changes in multiple files).
        Hide
        Claude Warren added a comment -

        A second attempt at the patch. This one seems to have worked better and only shows the differences.

        Show
        Claude Warren added a comment - A second attempt at the patch. This one seems to have worked better and only shows the differences.
        Hide
        Claude Warren added a comment -

        Test case for changes to Service.

        There was no existing Service test nor was there a package for it so I created TestService in the same package as the Service class, though I would expect that this will be in the test directory branch.

        Show
        Claude Warren added a comment - Test case for changes to Service. There was no existing Service test nor was there a package for it so I created TestService in the same package as the Service class, though I would expect that this will be in the test directory branch.
        Hide
        Rob Vesse added a comment -

        Thanks for cleaning the patch up and adding test cases for Service.

        I have applied the patch to Trunk so it should show up in the snapshots in the next day or so. Please test it once it is in Trunk to ensure it behaves as you expect.

        Show
        Rob Vesse added a comment - Thanks for cleaning the patch up and adding test cases for Service. I have applied the patch to Trunk so it should show up in the snapshots in the next day or so. Please test it once it is in Trunk to ensure it behaves as you expect.
        Hide
        Rob Vesse added a comment -

        Marking as resolved now patch is applied, will leave open for a few days to allow time for this to land in snapshots and get tested by others

        Show
        Rob Vesse added a comment - Marking as resolved now patch is applied, will leave open for a few days to allow time for this to land in snapshots and get tested by others
        Hide
        Hudson added a comment -

        Integrated in Jena_ARQ #571 (See https://builds.apache.org/job/Jena_ARQ/571/)
        Applied Claude Warren's patch for JENA-240 with some minor formatting tidy up, new tests pass as expected (Revision 1333529)

        Result = SUCCESS
        rvesse :
        Files :

        • /incubator/jena/Jena2/ARQ/trunk/src/main/java/com/hp/hpl/jena/sparql/engine/http/Service.java
        • /incubator/jena/Jena2/ARQ/trunk/src/test/java/com/hp/hpl/jena/sparql/engine/TS_Engine.java
        • /incubator/jena/Jena2/ARQ/trunk/src/test/java/com/hp/hpl/jena/sparql/engine/http
        • /incubator/jena/Jena2/ARQ/trunk/src/test/java/com/hp/hpl/jena/sparql/engine/http/TestService.java
        Show
        Hudson added a comment - Integrated in Jena_ARQ #571 (See https://builds.apache.org/job/Jena_ARQ/571/ ) Applied Claude Warren's patch for JENA-240 with some minor formatting tidy up, new tests pass as expected (Revision 1333529) Result = SUCCESS rvesse : Files : /incubator/jena/Jena2/ARQ/trunk/src/main/java/com/hp/hpl/jena/sparql/engine/http/Service.java /incubator/jena/Jena2/ARQ/trunk/src/test/java/com/hp/hpl/jena/sparql/engine/TS_Engine.java /incubator/jena/Jena2/ARQ/trunk/src/test/java/com/hp/hpl/jena/sparql/engine/http /incubator/jena/Jena2/ARQ/trunk/src/test/java/com/hp/hpl/jena/sparql/engine/http/TestService.java
        Hide
        Andy Seaborne added a comment - - edited

        Claude - TestService does not have the ASF grant flag set. Could you please re-attach with the flag set? Thanks.

        Show
        Andy Seaborne added a comment - - edited Claude - TestService does not have the ASF grant flag set. Could you please re-attach with the flag set? Thanks.
        Hide
        Andy Seaborne added a comment -

        If there were some documentation (plain text, or markdown) we could add it to the website. Javadoc can be hard to find.

        In fact a page on using federated would be good - if the features of Service are documented, I'll start a more general federated query page and include it.

        Show
        Andy Seaborne added a comment - If there were some documentation (plain text, or markdown) we could add it to the website. Javadoc can be hard to find. In fact a page on using federated would be good - if the features of Service are documented, I'll start a more general federated query page and include it.
        Hide
        Claude Warren added a comment -

        Reattached with ASF flag set

        Show
        Claude Warren added a comment - Reattached with ASF flag set
        Hide
        Claude Warren added a comment -

        An attempt at some documentation. Not sure how to submit it.

        Show
        Claude Warren added a comment - An attempt at some documentation. Not sure how to submit it.

          People

          • Assignee:
            Rob Vesse
            Reporter:
            Claude Warren
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 24h
              24h
              Remaining:
              Remaining Estimate - 24h
              24h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development