Lucene - Core
  1. Lucene - Core
  2. LUCENE-1484

Remove SegmentReader.document synchronization

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.9
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      This is probably the last synchronization issue in Lucene. It is the document method in SegmentReader. It is avoidable by using a threadlocal for FieldsReader.

      1. LUCENE-1484.patch
        10 kB
        Michael McCandless
      2. LUCENE-1484.patch
        8 kB
        Jason Rutherglen

        Activity

        Hide
        Jason Rutherglen added a comment -

        LUCENE-1484.patch

        • FieldsReader implements Cloneable
        • fieldsReaderLocal added to SegmentReader
        • TestIndexReader, TestFieldsReader, TestSegmentReader, TestParallelMultiSearcher passes
        Show
        Jason Rutherglen added a comment - LUCENE-1484 .patch FieldsReader implements Cloneable fieldsReaderLocal added to SegmentReader TestIndexReader, TestFieldsReader, TestSegmentReader, TestParallelMultiSearcher passes
        Hide
        Michael McCandless added a comment -

        Jason I'm seeing multiple test case failures in TestIndexReaderReopen with this patch. Are you seeing these?

        In general, can you say up front, on attaching a patch, whether it passes all tests for you, and if it doesn't, what the plan is for resolving the failures?

        Show
        Michael McCandless added a comment - Jason I'm seeing multiple test case failures in TestIndexReaderReopen with this patch. Are you seeing these? In general, can you say up front, on attaching a patch, whether it passes all tests for you, and if it doesn't, what the plan is for resolving the failures?
        Hide
        Michael McCandless added a comment -

        OK I've fixed a few issues with the patch. All tests & back-compat
        tests now pass. I think it's ready to commit. Jason can you review
        it?

        Details:

        • Fixed the bug causing test failures (we were not in fact cloning
          the FieldsReader in reopenSegment(), causing NPEs)
        • Put back the call to isDeleted() in SegmentReader.document – we
          lost the synchronization by inlining it.
        • Move fieldStreamTL.close() back out of the "if (isOriginal)" block
          in FieldsReader.close.
        • Put lost "private" back in front of a couple methods/members
        • Added javadocs
        • Removed dead code
        • Added CHANGES entry
        • Other small fixes...
        Show
        Michael McCandless added a comment - OK I've fixed a few issues with the patch. All tests & back-compat tests now pass. I think it's ready to commit. Jason can you review it? Details: Fixed the bug causing test failures (we were not in fact cloning the FieldsReader in reopenSegment(), causing NPEs) Put back the call to isDeleted() in SegmentReader.document – we lost the synchronization by inlining it. Move fieldStreamTL.close() back out of the "if (isOriginal)" block in FieldsReader.close. Put lost "private" back in front of a couple methods/members Added javadocs Removed dead code Added CHANGES entry Other small fixes...
        Hide
        Jason Rutherglen added a comment -

        It's in the ant script? I'll do this for the clone patch.

        On Sun, Dec 14, 2008 at 3:04 AM, Michael McCandless (JIRA)

        Show
        Jason Rutherglen added a comment - It's in the ant script? I'll do this for the clone patch. On Sun, Dec 14, 2008 at 3:04 AM, Michael McCandless (JIRA)
        Hide
        Michael McCandless added a comment -

        Jason I didn't understand your comment. Maybe this is the wrong issue?

        Show
        Michael McCandless added a comment - Jason I didn't understand your comment. Maybe this is the wrong issue?
        Hide
        Jason Rutherglen added a comment -

        The patch is OK.

        Found it "ant test-core". In the future I'll run this on the patches.

        Show
        Jason Rutherglen added a comment - The patch is OK. Found it "ant test-core". In the future I'll run this on the patches.
        Hide
        Michael McCandless added a comment -

        > Found it "ant test-core". In the future I'll run this on the patches.

        Actually it's best if you run "ant test" (runs contrib's unit tests, too), as well as "ant test-tag" (tests back compat).

        See http://wiki.apache.org/lucene-java/HowToContribute.

        Show
        Michael McCandless added a comment - > Found it "ant test-core". In the future I'll run this on the patches. Actually it's best if you run "ant test" (runs contrib's unit tests, too), as well as "ant test-tag" (tests back compat). See http://wiki.apache.org/lucene-java/HowToContribute .
        Hide
        Michael McCandless added a comment -

        Committed revision 727338. Thanks Jason!

        Show
        Michael McCandless added a comment - Committed revision 727338. Thanks Jason!
        Hide
        Jason Bennett added a comment -

        Is there any chance this patch could be released in 2.4.1, instead of waiting for 2.9?

        Show
        Jason Bennett added a comment - Is there any chance this patch could be released in 2.4.1, instead of waiting for 2.9?
        Hide
        Michael McCandless added a comment -

        Is there any chance this patch could be released in 2.4.1, instead of waiting for 2.9?

        Most likely not. It's really a new feature, and it touched a fair amount of code. We normally only backport bug fixes.

        Also, 2.4.0 has proven quite stable and I don't think at this point we're planning on a 2.4.1 release (at least it hasn't been discussed yet).

        I'm hoping, instead, that we can release 2.9 in not too much time.

        Show
        Michael McCandless added a comment - Is there any chance this patch could be released in 2.4.1, instead of waiting for 2.9? Most likely not. It's really a new feature, and it touched a fair amount of code. We normally only backport bug fixes. Also, 2.4.0 has proven quite stable and I don't think at this point we're planning on a 2.4.1 release (at least it hasn't been discussed yet). I'm hoping, instead, that we can release 2.9 in not too much time.

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Jason Rutherglen
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 96h
              96h
              Remaining:
              Remaining Estimate - 96h
              96h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development