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

Make SlowCompositeReaderWrapper constructor private

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.5, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      I found a couple of places in the code base that duplicate the logic of SlowCompositeReaderWrapper.wrap. I think SlowCompositeReaderWrapper.wrap (vs. new SlowCompositeReaderWrapper) is what users need so we should probably make SlowCompositeReaderWrapper constructor private to enforce usage of wrap.

      1. LUCENE-5187.patch
        16 kB
        Adrien Grand

        Activity

        Hide
        jpountz Adrien Grand added a comment -

        4.5 release -> bulk close

        Show
        jpountz Adrien Grand added a comment - 4.5 release -> bulk close
        Hide
        jpountz Adrien Grand added a comment -

        Thanks all for the review!

        Show
        jpountz Adrien Grand added a comment - Thanks all for the review!
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1517449 from Adrien Grand in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1517449 ]

        LUCENE-5187: Make SlowCompositeReaderWrapper's constructor private.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1517449 from Adrien Grand in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1517449 ] LUCENE-5187 : Make SlowCompositeReaderWrapper's constructor private.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1517447 from Adrien Grand in branch 'dev/trunk'
        [ https://svn.apache.org/r1517447 ]

        LUCENE-5187: Make SlowCompositeReaderWrapper's constructor private.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1517447 from Adrien Grand in branch 'dev/trunk' [ https://svn.apache.org/r1517447 ] LUCENE-5187 : Make SlowCompositeReaderWrapper's constructor private.
        Hide
        mikemccand Michael McCandless added a comment -

        +1 to commit, those grouping tests are fine if the wrapping doesn't happen (because the reader already happened to be atomic).

        Show
        mikemccand Michael McCandless added a comment - +1 to commit, those grouping tests are fine if the wrapping doesn't happen (because the reader already happened to be atomic).
        Hide
        thetaphi Uwe Schindler added a comment -

        Stroooong ++++++1

        I wanted to do that long time, but some tests were made me afraid.

        Show
        thetaphi Uwe Schindler added a comment - Stroooong ++++++1 I wanted to do that long time, but some tests were made me afraid.
        Hide
        rcmuir Robert Muir added a comment -

        My concern was the combination of the "intentional" comment and the fact it used the ctor (which forces wrapping always).

        I guess we can just see if it fails, and deal with it if so (by forcing compositeness).

        So I would just commit the patch for now!

        Show
        rcmuir Robert Muir added a comment - My concern was the combination of the "intentional" comment and the fact it used the ctor (which forces wrapping always). I guess we can just see if it fails, and deal with it if so (by forcing compositeness). So I would just commit the patch for now!
        Hide
        jpountz Adrien Grand added a comment -

        Hmm, my understanding was that this test just needs a top-level fieldcache instance for testing purpose but I may be wrong...

        Show
        jpountz Adrien Grand added a comment - Hmm, my understanding was that this test just needs a top-level fieldcache instance for testing purpose but I may be wrong...
        Hide
        rcmuir Robert Muir added a comment -

        +1

        In the case of grouping tests, is it really safe to change this way?

               // NOTE: intentional but temporary field cache insanity!
        -      final FieldCache.Ints docIdToFieldId = FieldCache.DEFAULT.getInts(new SlowCompositeReaderWrapper(r), "id", false);
        +      final FieldCache.Ints docIdToFieldId = FieldCache.DEFAULT.getInts(SlowCompositeReaderWrapper.wrap(r), "id", false);
        

        if needed, Maybe instead, we should force compositeness by e.g. inserting an empty reader if necessary or whatever. but i'm not even sure what this test is doing.

        Show
        rcmuir Robert Muir added a comment - +1 In the case of grouping tests, is it really safe to change this way? // NOTE: intentional but temporary field cache insanity! - final FieldCache.Ints docIdToFieldId = FieldCache.DEFAULT.getInts(new SlowCompositeReaderWrapper(r), "id", false); + final FieldCache.Ints docIdToFieldId = FieldCache.DEFAULT.getInts(SlowCompositeReaderWrapper.wrap(r), "id", false); if needed, Maybe instead, we should force compositeness by e.g. inserting an empty reader if necessary or whatever. but i'm not even sure what this test is doing.
        Hide
        jpountz Adrien Grand added a comment -

        Here is a patch.

        Show
        jpountz Adrien Grand added a comment - Here is a patch.

          People

          • Assignee:
            jpountz Adrien Grand
            Reporter:
            jpountz Adrien Grand
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development