Lucene - Core
  1. Lucene - Core
  2. LUCENE-3630

MultiReader and ParallelReader accidently override doOpenIfChanged(boolean readOnly) with doOpenIfChanged(boolean doClone)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.5
    • Fix Version/s: 3.6
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      I found this during adding deprecations for RW access in LUCENE-3606:

      the base class defines doOpenIfChanged(boolean readOnly), but MultiReader and ParallelReader "override" this method with a signature doOpenIfChanged(doClone) and missing @Override. This makes consumers calling IR.openIfChanged(boolean readOnly) do the wrong thing. Instead they should get UOE like for the other unimplemented doOpenIfChanged methods in MR and PR.

      Easy fix is to rename and hide this internal "reopen" method, like DirectoryReader,...

      1. LUCENE-3630.patch
        2 kB
        Uwe Schindler
      2. LUCENE-3630-addMissingMethod.patch
        2 kB
        Uwe Schindler
      3. LUCENE-3630-revert.patch
        7 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Uwe Schindler added a comment -

          Only MultiReader has this problem, ParallelReader used another method name for the internal reopen. I will make the internal implementation private for both classes (they were protected because of the above override issue) and rename it to doReopen().

          The same on trunk, where some relicts of those method signatures are still protected. But the bug does not occur here.

          Show
          Uwe Schindler added a comment - Only MultiReader has this problem, ParallelReader used another method name for the internal reopen. I will make the internal implementation private for both classes (they were protected because of the above override issue) and rename it to doReopen(). The same on trunk, where some relicts of those method signatures are still protected. But the bug does not occur here.
          Hide
          Uwe Schindler added a comment -

          Committed 3.x revision: 1212754
          Committed changes merge + method renaming to trunk revision: 1212755

          Show
          Uwe Schindler added a comment - Committed 3.x revision: 1212754 Committed changes merge + method renaming to trunk revision: 1212755
          Hide
          Uwe Schindler added a comment -

          I found also a bug report on the user list about reopen not working with MultiReader. The use was calling openIfChanged(true) on MultiReader -> he got a clone instead of a reopened reader. The fix is to also implement doOpenIfChanged(boolean openReadOnly), but allow only true as param and throw UOE on false.

          Show
          Uwe Schindler added a comment - I found also a bug report on the user list about reopen not working with MultiReader. The use was calling openIfChanged(true) on MultiReader -> he got a clone instead of a reopened reader. The fix is to also implement doOpenIfChanged(boolean openReadOnly), but allow only true as param and throw UOE on false.
          Hide
          Uwe Schindler added a comment -

          Added missing doOpenIfChanged(readonly=true) support in 3.x revision: 1212756

          Show
          Uwe Schindler added a comment - Added missing doOpenIfChanged(readonly=true) support in 3.x revision: 1212756
          Hide
          Uwe Schindler added a comment -

          Further investigation showed that supporting openReadOnly on clone/readonly f*cks up refcounting. So don't support it at all and revert the last commit, clearly saying this.

          Show
          Uwe Schindler added a comment - Further investigation showed that supporting openReadOnly on clone/readonly f*cks up refcounting. So don't support it at all and revert the last commit, clearly saying this.
          Hide
          Uwe Schindler added a comment -

          Patch with revert and more explanatory UOE for clone and doOpenIfChanged

          Show
          Uwe Schindler added a comment - Patch with revert and more explanatory UOE for clone and doOpenIfChanged
          Hide
          Uwe Schindler added a comment -

          Committed fix in revision: 1212787

          Show
          Uwe Schindler added a comment - Committed fix in revision: 1212787

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Uwe Schindler
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development