Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-3630

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

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 3.5
    • 3.6
    • core/index
    • None
    • 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,...

      Attachments

        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

            uschindler 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.

            uschindler 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.
            uschindler Uwe Schindler added a comment -

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

            uschindler Uwe Schindler added a comment - Committed 3.x revision: 1212754 Committed changes merge + method renaming to trunk revision: 1212755
            uschindler 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.

            uschindler 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.
            uschindler Uwe Schindler added a comment -

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

            uschindler Uwe Schindler added a comment - Added missing doOpenIfChanged(readonly=true) support in 3.x revision: 1212756
            uschindler 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.

            uschindler 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.
            uschindler Uwe Schindler added a comment -

            Patch with revert and more explanatory UOE for clone and doOpenIfChanged

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

            Committed fix in revision: 1212787

            uschindler Uwe Schindler added a comment - Committed fix in revision: 1212787
            tomoko Tomoko Uchida added a comment -

            This issue was moved to GitHub issue: #4704.

            tomoko Tomoko Uchida added a comment - This issue was moved to GitHub issue: #4704 .

            People

              uschindler Uwe Schindler
              uschindler Uwe Schindler
              Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: