Lucene - Core
  1. Lucene - Core
  2. LUCENE-3464

Rename IndexReader.reopen to make it clear that reopen may not happen

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.5, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Spinoff from LUCENE-3454 where Shai noted this inconsistency.

      IR.reopen sounds like an unconditional operation, which has trapped users in the past into always closing the old reader instead of only closing it if the returned reader is new.

      I think this hidden maybe-ness is trappy and we should rename it (maybeReopen? reopenIfNeeded?).

      In addition, instead of returning "this" when the reopen didn't happen, I think we should return null to enforce proper usage of the maybe-ness of this API.

      1. LUCENE-3464.patch
        51 kB
        Michael McCandless
      2. LUCENE-3464.patch
        60 kB
        Michael McCandless
      3. LUCENE-3464.3x.patch
        58 kB
        Michael McCandless
      4. LUCENE-3464_see_its_just_fine.patch
        2 kB
        Robert Muir

        Activity

        Hide
        Uwe Schindler added a comment -

        Bulk close after release of 3.5

        Show
        Uwe Schindler added a comment - Bulk close after release of 3.5
        Hide
        Uwe Schindler added a comment -

        Committed 3.x revision: 1204970
        Merged to trunk revision: 1204971

        Show
        Uwe Schindler added a comment - Committed 3.x revision: 1204970 Merged to trunk revision: 1204971
        Hide
        Uwe Schindler added a comment -

        I will add a comment to the javadocs with a simple statement:

         * <p><b>Note:</b> The default implementation of {@link FilterIndexReader#doOpenIfChanged}
         * throws {@link UnsupportedOperationException} (like the base class),
         * so it's not possible to reopen a <code>FilterIndexReader</code>.
         * To reopen, you have to first reopen the underlying reader
         * and wrap it again with the custom filter.
        

        In this case the responsibility is moved over to the consumer. Its the same like with SlowMultiReaderWrapper: It does not support reopen, to support this, reopen the underlying reader and make it slow again

        Show
        Uwe Schindler added a comment - I will add a comment to the javadocs with a simple statement: * <p><b>Note:</b> The default implementation of {@link FilterIndexReader#doOpenIfChanged} * throws {@link UnsupportedOperationException} (like the base class), * so it's not possible to reopen a <code>FilterIndexReader</code>. * To reopen, you have to first reopen the underlying reader * and wrap it again with the custom filter. In this case the responsibility is moved over to the consumer. Its the same like with SlowMultiReaderWrapper: It does not support reopen, to support this, reopen the underlying reader and make it slow again
        Hide
        Robert Muir added a comment -

        And just again: I want to point out this is the same fundamental problem as LUCENE-2828.

        This is why abstract classes are NOT the solution to backwards compatibility, because
        delegators over abstract classes get all jacked up.

        Show
        Robert Muir added a comment - And just again: I want to point out this is the same fundamental problem as LUCENE-2828 . This is why abstract classes are NOT the solution to backwards compatibility, because delegators over abstract classes get all jacked up.
        Hide
        Robert Muir added a comment -

        Not in your proof of concept. And this proof of concept is no proof as it modifies FilteredIndexReader, so it would not work with 3.5.0RC1.

        I'm not arguing that nothing needs to be done: I'm just saying that the fact its protected isnt really a problem.

        For 3.5.0 there is already a workaround.

        Show
        Robert Muir added a comment - Not in your proof of concept. And this proof of concept is no proof as it modifies FilteredIndexReader, so it would not work with 3.5.0RC1. I'm not arguing that nothing needs to be done: I'm just saying that the fact its protected isnt really a problem. For 3.5.0 there is already a workaround.
        Hide
        Uwe Schindler added a comment -

        As I said before, this is simple, FIR's impl only delegates if its overriden. otherwise throws UOE.

        Not in your proof of concept. And this proof of concept is no proof as it modifies FilteredIndexReader, so it would not work with 3.5.0RC1.

        Please note: For the above reason: FilterIndexReader in the past never delegated reopen() - for this exact reason (there was an issue open why delegating is wrong - have to look it up).

        Show
        Uwe Schindler added a comment - As I said before, this is simple, FIR's impl only delegates if its overriden. otherwise throws UOE. Not in your proof of concept. And this proof of concept is no proof as it modifies FilteredIndexReader, so it would not work with 3.5.0RC1. Please note: For the above reason: FilterIndexReader in the past never delegated reopen() - for this exact reason (there was an issue open why delegating is wrong - have to look it up).
        Hide
        Robert Muir added a comment -

        As I said before, this is simple, FIR's impl only delegates if its overriden. otherwise throws UOE.

        Show
        Robert Muir added a comment - As I said before, this is simple, FIR's impl only delegates if its overriden. otherwise throws UOE.
        Hide
        Uwe Schindler added a comment -

        Robert: If you wrap a standard SegmentReader, it supports doOpenIfChanged. If I wrap it with my own custom FilterIndexReader that does not implement doOpenIfChanged, it will silently pass the return value of SegmentReader.doOpenIfChanged() which is no longer filtered. By throwing UOE in the default FilterIndexReader the user will see this when he tries to reopen.

        Show
        Uwe Schindler added a comment - Robert: If you wrap a standard SegmentReader, it supports doOpenIfChanged. If I wrap it with my own custom FilterIndexReader that does not implement doOpenIfChanged, it will silently pass the return value of SegmentReader.doOpenIfChanged() which is no longer filtered. By throwing UOE in the default FilterIndexReader the user will see this when he tries to reopen.
        Hide
        Uwe Schindler added a comment -

        Robert read my comment about the reason why FilterIndexReader should never delegate to doOpenIfChanged(), as it would return an unfiltered reader on reopening, which would be a hidden bug for an implementor!

        Show
        Uwe Schindler added a comment - Robert read my comment about the reason why FilterIndexReader should never delegate to doOpenIfChanged(), as it would return an unfiltered reader on reopening, which would be a hidden bug for an implementor!
        Hide
        Robert Muir added a comment -

        In my patch FIR returns whatever the underlying reader does.

        If you want this to be UOE if doReopen is not overridden, well, you know how to do that

        Show
        Robert Muir added a comment - In my patch FIR returns whatever the underlying reader does. If you want this to be UOE if doReopen is not overridden , well, you know how to do that
        Hide
        Uwe Schindler added a comment -

        Thats no problem, it just calls super.doOpenIfChanged, see my proof of concept.

        This throws UOE as FilterIndexReader (correctly!!!) does not implement doOpenIfChanged. If it would implement if a FilterIndexReader without any doOpenIfChanged impl would be horribly broken as it would return an unfiltered reader!!!

        Show
        Uwe Schindler added a comment - Thats no problem, it just calls super.doOpenIfChanged, see my proof of concept. This throws UOE as FilterIndexReader (correctly!!!) does not implement doOpenIfChanged. If it would implement if a FilterIndexReader without any doOpenIfChanged impl would be horribly broken as it would return an unfiltered reader!!!
        Hide
        Robert Muir added a comment -

        Uwe, see my patch, it uses its own sophisticated packaged and works fine.

        Show
        Robert Muir added a comment - Uwe, see my patch, it uses its own sophisticated packaged and works fine.
        Hide
        Uwe Schindler added a comment -

        This code does not compile (fails by saying IndexReader has no accessible method doOpenIfChanged()):

        package my.sophisticated.package;
        
        @Override
        protected IndexReader doOpenIfChanged() throws CorruptIndexException, IOException {
          final IndexReader n=in.doOpenIfChanged();
          return (n==null) ? null : new MySophisticatedIndexReader(n);
        }
        

        This is the working workaround but looks wrong (and works around the VirtualMethod issues):

        @Override
        protected IndexReader doOpenIfChanged() throws CorruptIndexException, IOException {
          final IndexReader n=IndexReader.openIfChanged(in);
          return (n==null) ? null : new MySophisticatedIndexReader(n);
        }
        
        Show
        Uwe Schindler added a comment - This code does not compile (fails by saying IndexReader has no accessible method doOpenIfChanged()): package my.sophisticated. package ; @Override protected IndexReader doOpenIfChanged() throws CorruptIndexException, IOException { final IndexReader n=in.doOpenIfChanged(); return (n== null ) ? null : new MySophisticatedIndexReader(n); } This is the working workaround but looks wrong (and works around the VirtualMethod issues): @Override protected IndexReader doOpenIfChanged() throws CorruptIndexException, IOException { final IndexReader n=IndexReader.openIfChanged(in); return (n== null ) ? null : new MySophisticatedIndexReader(n); }
        Hide
        Robert Muir added a comment -

        Thats no problem, it just calls super.doOpenIfChanged, see my proof of concept.

        Show
        Robert Muir added a comment - Thats no problem, it just calls super.doOpenIfChanged, see my proof of concept.
        Hide
        Simon Willnauer added a comment -

        I don't understand this statement, because FilterReader extends IndexReader so it should be able to call protected IR methods.

        but not the delegates method though.

        Show
        Simon Willnauer added a comment - I don't understand this statement, because FilterReader extends IndexReader so it should be able to call protected IR methods. but not the delegates method though.
        Hide
        Robert Muir added a comment -

        The problem: This cannot be implemented if the custom Filter is in a 3rd party package, as it cannot call the protected doOpenIfChanged.

        I don't understand this statement, because FilterReader extends IndexReader so it should be able to call protected IR methods.

        Show
        Robert Muir added a comment - The problem: This cannot be implemented if the custom Filter is in a 3rd party package, as it cannot call the protected doOpenIfChanged. I don't understand this statement, because FilterReader extends IndexReader so it should be able to call protected IR methods.
        Hide
        Uwe Schindler added a comment - - edited

        I am reopening this issue as there is some API problem which makes openIfChanged not consistently useable with FilterIndexReader:

        If you have a FilterIndexReader that did in the past not implement reopen(...), the base class IndexReader throwed UOE. This was fine, as a FilterIndexReader cannot support reopen unless specifically supported. A FilterIndexReader of course must reopen the delegate reader and then wrap it again to Filter. This was done by overriding reopen() methods, checking if the delegate returned another reader and if yes, wrapping it.

        I tried to transform code that implement this pattern to Lucene 3.5RC1 but failed to do it in the clean way: Reopen was replaced by a static IR.openIfChanged(IR oldReader) that delegates to the specific IndexReaders implementation of doOpenIfChanged (which is protected).

        To implement the above pattern, doOpenIfChanged must be overridden in FilterIndexReader (again the default must thorw UOE, otherwise reopening a filtered reader returns a non-filtered one). This method must call delegate's doOpenIfChanged and if it returns != null, wrap by our FilterIndexReader implementation. The problem: This cannot be implemented if the custom Filter is in a 3rd party package, as it cannot call the protected doOpenIfChanged. The workaround is to use IndexReader.openIfChanged(delegate), but this look borken and violates the pattern.

        The good thing with the workaround is that the VirtualMethod sophisticated backwards works correctly. We must at least document this behaviour in FilterIndexReader or fix the API.

        Show
        Uwe Schindler added a comment - - edited I am reopening this issue as there is some API problem which makes openIfChanged not consistently useable with FilterIndexReader: If you have a FilterIndexReader that did in the past not implement reopen(...), the base class IndexReader throwed UOE. This was fine, as a FilterIndexReader cannot support reopen unless specifically supported. A FilterIndexReader of course must reopen the delegate reader and then wrap it again to Filter. This was done by overriding reopen() methods, checking if the delegate returned another reader and if yes, wrapping it. I tried to transform code that implement this pattern to Lucene 3.5RC1 but failed to do it in the clean way: Reopen was replaced by a static IR.openIfChanged(IR oldReader) that delegates to the specific IndexReaders implementation of doOpenIfChanged (which is protected). To implement the above pattern, doOpenIfChanged must be overridden in FilterIndexReader (again the default must thorw UOE, otherwise reopening a filtered reader returns a non-filtered one). This method must call delegate's doOpenIfChanged and if it returns != null, wrap by our FilterIndexReader implementation. The problem: This cannot be implemented if the custom Filter is in a 3rd party package, as it cannot call the protected doOpenIfChanged. The workaround is to use IndexReader.openIfChanged(delegate), but this look borken and violates the pattern. The good thing with the workaround is that the VirtualMethod sophisticated backwards works correctly. We must at least document this behaviour in FilterIndexReader or fix the API.
        Hide
        Michael McCandless added a comment -

        Thanks everyone!

        Show
        Michael McCandless added a comment - Thanks everyone!
        Hide
        Uwe Schindler added a comment - - edited

        This sophisticated backwards seems fine.

        The most common use-case is:
        One has a FilterIndexReader. All FilterIndexReaders should override reopen(), as the reopened reader has to be wrapped, too. In that case the filter would call super.reopen (or delegate.reopen), this would then trigger backwards. IndexReader.openIfChanged() would also work on this filtered reader, as it would detect the FilterIR to be old-style and would call its reopen method (so the call chain would be: IR.openIfChanged(filter) -> filter.reopen() -> wrappedReader.reopen() -> IR.openIfChanged(wrappedReader)) [wrappedReader is new-style in this example]

        I have to think about other use-cases, but the most common one seems to be fine.

        Show
        Uwe Schindler added a comment - - edited This sophisticated backwards seems fine. The most common use-case is: One has a FilterIndexReader. All FilterIndexReaders should override reopen(), as the reopened reader has to be wrapped, too. In that case the filter would call super.reopen (or delegate.reopen), this would then trigger backwards. IndexReader.openIfChanged() would also work on this filtered reader, as it would detect the FilterIR to be old-style and would call its reopen method (so the call chain would be: IR.openIfChanged(filter) -> filter.reopen() -> wrappedReader.reopen() -> IR.openIfChanged(wrappedReader)) [wrappedReader is new-style in this example] I have to think about other use-cases, but the most common one seems to be fine.
        Hide
        Michael McCandless added a comment -

        Tentative 3.x patch.

        I had to whip out VirtualMethod for back compat.... would appreciate a review that I did this correctly! Heavy guns.

        Show
        Michael McCandless added a comment - Tentative 3.x patch. I had to whip out VirtualMethod for back compat.... would appreciate a review that I did this correctly! Heavy guns.
        Hide
        Michael McCandless added a comment -

        New patch, cutting over to IndexReader.openIfChanged(oldReader) static
        methods... I like this much better since it in no way implies the old
        reader is reopened / altered in place. It returns null if there is no
        change to the index. I think it's ready!

        Show
        Michael McCandless added a comment - New patch, cutting over to IndexReader.openIfChanged(oldReader) static methods... I like this much better since it in no way implies the old reader is reopened / altered in place. It returns null if there is no change to the index. I think it's ready!
        Hide
        Ryan McKinley added a comment -

        the method implies that the reopen will happen "in place". And I've seen users try to simply do IR.reopen().

        funny, that's what i thought it did!

        If you have to use the results value, should it be:
        getFreshReader(oldReader)
        or something?

        without the 'get' it seems like it operates on the reader itself, not the return value.

        Show
        Ryan McKinley added a comment - the method implies that the reopen will happen "in place". And I've seen users try to simply do IR.reopen(). funny, that's what i thought it did! If you have to use the results value, should it be: getFreshReader(oldReader) or something? without the 'get' it seems like it operates on the reader itself, not the return value.
        Hide
        Michael McCandless added a comment -

        Patch, changing to .reopenIfChanged.

        Still wondering about IR.openIfChanged though....

        Show
        Michael McCandless added a comment - Patch, changing to .reopenIfChanged. Still wondering about IR.openIfChanged though....
        Hide
        Michael McCandless added a comment -

        Another trappiness I've seen on IR.reopen is... the method implies that the reopen will happen "in place". And I've seen users try to simply do IR.reopen().

        Maybe, the public API we expose should be something like IR.openIfChanged(oldReader), a static method? This would open a new reader only if there are changes vs the old one, and would try to share resources (ie shared segments) with the old reader, when possible.

        We'd have to add protected IR methods that "really" try to do the reopen.

        Show
        Michael McCandless added a comment - Another trappiness I've seen on IR.reopen is... the method implies that the reopen will happen "in place". And I've seen users try to simply do IR.reopen(). Maybe, the public API we expose should be something like IR.openIfChanged(oldReader), a static method? This would open a new reader only if there are changes vs the old one, and would try to share resources (ie shared segments) with the old reader, when possible. We'd have to add protected IR methods that "really" try to do the reopen.
        Hide
        Michael McCandless added a comment -

        Can we just make it final when we backport this change?

        Duh no we can't since we have core IR impls subclassing IR...

        Show
        Michael McCandless added a comment - Can we just make it final when we backport this change? Duh no we can't since we have core IR impls subclassing IR...
        Hide
        Michael McCandless added a comment -

        I like reopenIfChanged too!

        unfortunately, reopen() is not final in IR and uses doReopen,

        Can we just make it final when we backport this change? It's very expert to externally subclass IR and override reopen...

        Show
        Michael McCandless added a comment - I like reopenIfChanged too! unfortunately, reopen() is not final in IR and uses doReopen, Can we just make it final when we backport this change? It's very expert to externally subclass IR and override reopen...
        Hide
        Uwe Schindler added a comment -

        We can even do the change in 3.x, if we keep the old method delegating to the new one. In 3.x we also need some sophisticated VirtualMethod usage then, as we have to take care of custom IR implementations that may only override the old one - unfortunately, reopen() is not final in IR and uses doReopen, like the other methods.

        reopenIfChanged() - my favourite!

        Show
        Uwe Schindler added a comment - We can even do the change in 3.x, if we keep the old method delegating to the new one. In 3.x we also need some sophisticated VirtualMethod usage then, as we have to take care of custom IR implementations that may only override the old one - unfortunately, reopen() is not final in IR and uses doReopen, like the other methods. reopenIfChanged() - my favourite!
        Hide
        Ryan McKinley added a comment -

        maybe:
        reopenIfChanged()

        reopenOnChange suggests, it will reopen when something changes.

        Show
        Ryan McKinley added a comment - maybe: reopenIfChanged() reopenOnChange suggests, it will reopen when something changes.
        Hide
        Doron Cohen added a comment -

        I liked reopen()... (but also like returning null in case there's nothing newer...)

        If the name is going to change, two additional names to consider:

        • newest()
        • newer()

        For "newest()" I think current behavior of returning "this" makes sense when "this" is the newest.
        For "newer()" returning null in that case seems right.

        One problem I have with these names is that they both seem to hide the fact that things are going on down there, when it is required to open a new reader...

        Show
        Doron Cohen added a comment - I liked reopen()... (but also like returning null in case there's nothing newer...) If the name is going to change, two additional names to consider: newest() newer() For "newest()" I think current behavior of returning "this" makes sense when "this" is the newest. For "newer()" returning null in that case seems right. One problem I have with these names is that they both seem to hide the fact that things are going on down there, when it is required to open a new reader...
        Hide
        Martijn van Groningen added a comment -

        +1 reopenOnChange()

        Names that contain maybe or try sound vague to me as a user.

        Show
        Martijn van Groningen added a comment - +1 reopenOnChange() Names that contain maybe or try sound vague to me as a user.
        Hide
        Simon Willnauer added a comment -

        just a couple of variants:

        tryReopen()
        reopenOnChange()

        just and idea, we could also have a higherlevel api here on IR that refreshes the reader something like IR IR#refresh(IR prefious, boolean closePrevious)

        Show
        Simon Willnauer added a comment - just a couple of variants: tryReopen() reopenOnChange() just and idea, we could also have a higherlevel api here on IR that refreshes the reader something like IR IR#refresh(IR prefious, boolean closePrevious)
        Hide
        Mark Miller added a comment -

        reopenIfNeeded

        +1 - can't think of anything better yet

        Show
        Mark Miller added a comment - reopenIfNeeded +1 - can't think of anything better yet

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development