Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-8190

Implement Closeable on TupleStream

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Implemented
    • Affects Version/s: 6.0
    • Fix Version/s: 6.0
    • Component/s: SolrJ
    • Labels:
      None

      Description

      Implementing Closeable on TupleStream provides the ability to use try-with-resources (https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html) in tests and in practice. This prevents TupleStreams from being left open when there is an error in the tests.

      1. SOLR-8190.patch
        1 kB
        Kevin Risden
      2. SOLR-8190.patch
        51 kB
        Jason Gerlowski
      3. SOLR-8190.patch
        55 kB
        Kevin Risden

        Issue Links

          Activity

          Hide
          risdenk Kevin Risden added a comment -

          Found that CloudSolrStream yields two NullPointerExceptions when being closed during tests for solrStreams and cloudSolrClient being null. Filed SOLR-8191 to address it.

          Show
          risdenk Kevin Risden added a comment - Found that CloudSolrStream yields two NullPointerExceptions when being closed during tests for solrStreams and cloudSolrClient being null. Filed SOLR-8191 to address it.
          Hide
          risdenk Kevin Risden added a comment -

          Fixed stream tests to use assertEquals methods instead of assertTrue(boolean condition) since most of the conditions were for equals.

          Show
          risdenk Kevin Risden added a comment - Fixed stream tests to use assertEquals methods instead of assertTrue(boolean condition) since most of the conditions were for equals.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Would be nice to get this rolling again. To keep it up to date, I've updated the patch to apply cleanly off of trunk.

          Tests still fail due to the NPE addressed in the (unresolved) SOLR-8091.

          Show
          gerlowskija Jason Gerlowski added a comment - Would be nice to get this rolling again. To keep it up to date, I've updated the patch to apply cleanly off of trunk. Tests still fail due to the NPE addressed in the (unresolved) SOLR-8091 .
          Hide
          risdenk Kevin Risden added a comment -

          Jason Gerlowski Thanks for taking a look. I think the above JIRA should have been SOLR-8191.

          Show
          risdenk Kevin Risden added a comment - Jason Gerlowski Thanks for taking a look. I think the above JIRA should have been SOLR-8191 .
          Hide
          joel.bernstein Joel Bernstein added a comment -

          Gave this a quick review. I like the idea of implementing closable, but in this case close() would get called twice for each test.

          Maybe the better approach is to call close() in the finally block of the methods that call open().

          Show
          joel.bernstein Joel Bernstein added a comment - Gave this a quick review. I like the idea of implementing closable, but in this case close() would get called twice for each test. Maybe the better approach is to call close() in the finally block of the methods that call open().
          Hide
          risdenk Kevin Risden added a comment -

          The getTuples method was added recently which is the only place that open/close is called. This is where the try w/ resources should go instead of wrapping each test. I'll update the patch with changes. This should make the patch a lot simpler. While investigating this I found that there are now two new NPE issues w/ FacetStream and StatsStream with the same type cause as in SOLR-8191.

          Show
          risdenk Kevin Risden added a comment - The getTuples method was added recently which is the only place that open/close is called. This is where the try w/ resources should go instead of wrapping each test. I'll update the patch with changes. This should make the patch a lot simpler. While investigating this I found that there are now two new NPE issues w/ FacetStream and StatsStream with the same type cause as in SOLR-8191 .
          Hide
          gerlowskija Jason Gerlowski added a comment -

          +1 for pushing the try-with-resources into the getTuples method. I was considering this when I updated the patch, but I didn't want to step on any toes (especially since I'm not super familiar the code) so I just left as-is.

          Should really shrink the patch too, which is a nice plus.

          Show
          gerlowskija Jason Gerlowski added a comment - +1 for pushing the try-with-resources into the getTuples method. I was considering this when I updated the patch, but I didn't want to step on any toes (especially since I'm not super familiar the code) so I just left as-is. Should really shrink the patch too, which is a nice plus.
          Hide
          risdenk Kevin Risden added a comment -

          So I put more thought into this last night and since TupleStream has an open method it makes try-with-resources not really applicable. In the case here, it will call close twice as implemented. try-with-resources can't be pushed into getTuples since try-with-resources doesn't work with a open method.

          Thinking about this brought up the following thoughts:

          • What should happen when open is called twice?
          • What should happen when close is called twice?
          • What should happen when close is called without open being called?
          • Are there places in the code where open/close is called without a try/finally? Will that cause issues?
          • Are there places in the code where TupleStream.open is called without a related close call?

          There are no checks currently to see if a stream has already been opened or closed. This is what is causing the different NPE exceptions like in SOLR-8191.

          For this ticket, I think just implementing Closeable on TupleStream and not changing the tests is appropriate. The above items should be addressed though. This will make the patch smaller and the tests can be improved in followup JIRAS.

          Joel Bernstein/Jason Gerlowski - Thoughts on the above?

          Show
          risdenk Kevin Risden added a comment - So I put more thought into this last night and since TupleStream has an open method it makes try-with-resources not really applicable. In the case here, it will call close twice as implemented. try-with-resources can't be pushed into getTuples since try-with-resources doesn't work with a open method. Thinking about this brought up the following thoughts: What should happen when open is called twice? What should happen when close is called twice? What should happen when close is called without open being called? Are there places in the code where open/close is called without a try/finally? Will that cause issues? Are there places in the code where TupleStream.open is called without a related close call? There are no checks currently to see if a stream has already been opened or closed. This is what is causing the different NPE exceptions like in SOLR-8191 . For this ticket, I think just implementing Closeable on TupleStream and not changing the tests is appropriate. The above items should be addressed though. This will make the patch smaller and the tests can be improved in followup JIRAS. Joel Bernstein / Jason Gerlowski - Thoughts on the above?
          Hide
          joel.bernstein Joel Bernstein added a comment -

          The /stream and /sql handler should be calling open() and close() in the majority of situations. There are also situations where the Streams themselves may open() and close() internal streams. For example the hashJoin stream may open a stream read it into a hashtable and then close the stream.

          But if people want to work directly with the Streaming API, rather the sending a Streaming Expression to the /stream handler, it would be nice to add some robustness to how open and close are handled. Probably we should through exceptions in the first three cases you mention.

          Show
          joel.bernstein Joel Bernstein added a comment - The /stream and /sql handler should be calling open() and close() in the majority of situations. There are also situations where the Streams themselves may open() and close() internal streams. For example the hashJoin stream may open a stream read it into a hashtable and then close the stream. But if people want to work directly with the Streaming API, rather the sending a Streaming Expression to the /stream handler, it would be nice to add some robustness to how open and close are handled. Probably we should through exceptions in the first three cases you mention.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Yeah, I guess it doesn't make a ton of sense to push try-with-finally when it doesn't really work with {{TupleStream}}s API.

          I agree with you and Joel, it makes sense to catch these special cases and throw an IllegalStateException or something similar.

          Show
          gerlowskija Jason Gerlowski added a comment - Yeah, I guess it doesn't make a ton of sense to push try-with-finally when it doesn't really work with {{TupleStream}}s API. I agree with you and Joel, it makes sense to catch these special cases and throw an IllegalStateException or something similar.
          Hide
          risdenk Kevin Risden added a comment -

          Added simple patch for just implementing Closeable on TupleStream.

          Show
          risdenk Kevin Risden added a comment - Added simple patch for just implementing Closeable on TupleStream.
          Hide
          risdenk Kevin Risden added a comment -

          Joel Bernstein - Can the latest simple patch be reviewed?

          Show
          risdenk Kevin Risden added a comment - Joel Bernstein - Can the latest simple patch be reviewed?
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 739b81063eb9045b4686ce8ad702c61451503306 in lucene-solr's branch refs/heads/master from jbernste
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=739b810 ]

          SOLR-8190: Implement Closeable on TupleStream

          Show
          jira-bot ASF subversion and git services added a comment - Commit 739b81063eb9045b4686ce8ad702c61451503306 in lucene-solr's branch refs/heads/master from jbernste [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=739b810 ] SOLR-8190 : Implement Closeable on TupleStream

            People

            • Assignee:
              joel.bernstein Joel Bernstein
              Reporter:
              risdenk Kevin Risden
            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development