Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1
    • Fix Version/s: 2.9
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Based on discussion http://www.nabble.com/IndexReader.reopen-issue-td18070256.html. The problem is reopen returns the same reader if there are no changes, so if docs are deleted from the new reader, they are also reflected in the previous reader which is not always desired behavior.

      1. lucene-1314.patch
        44 kB
        Jason Rutherglen
      2. lucene-1314.patch
        41 kB
        Jason Rutherglen
      3. lucene-1314.patch
        39 kB
        Jason Rutherglen
      4. lucene-1314.patch
        39 kB
        Jason Rutherglen
      5. lucene-1314.patch
        29 kB
        Jason Rutherglen
      6. lucene-1314.patch
        35 kB
        Jason Rutherglen
      7. lucene-1314.patch
        20 kB
        Michael McCandless
      8. lucene-1314.patch
        21 kB
        Jason Rutherglen
      9. lucene-1314.patch
        21 kB
        Jason Rutherglen
      10. lucene-1314.patch
        16 kB
        Jason Rutherglen
      11. lucene-1314.patch
        16 kB
        Jason Rutherglen
      12. lucene-1314.patch
        13 kB
        Jason Rutherglen
      13. LUCENE-1314.patch
        14 kB
        Michael McCandless
      14. LUCENE-1314.patch
        0.9 kB
        Jason Rutherglen
      15. LUCENE-1314.patch
        4 kB
        Jason Rutherglen
      16. LUCENE-1314.patch
        3 kB
        Michael McCandless
      17. LUCENE-1314.patch
        1 kB
        Jason Rutherglen
      18. LUCENE-1314.patch
        91 kB
        Michael McCandless
      19. LUCENE-1314.patch
        90 kB
        Michael McCandless
      20. LUCENE-1314.patch
        82 kB
        Jason Rutherglen
      21. LUCENE-1314.patch
        82 kB
        Jason Rutherglen
      22. LUCENE-1314.patch
        82 kB
        Jason Rutherglen
      23. LUCENE-1314.patch
        82 kB
        Jason Rutherglen
      24. LUCENE-1314.patch
        82 kB
        Jason Rutherglen
      25. LUCENE-1314.patch
        61 kB
        Jason Rutherglen
      26. LUCENE-1314.patch
        76 kB
        Michael McCandless
      27. LUCENE-1314.patch
        49 kB
        Jason Rutherglen
      28. LUCENE-1314.patch
        48 kB
        Jason Rutherglen
      29. LUCENE-1314.patch
        45 kB
        Jason Rutherglen
      30. LUCENE-1314.patch
        41 kB
        Jason Rutherglen
      31. LUCENE-1314.patch
        40 kB
        Jason Rutherglen
      32. LUCENE-1314.patch
        40 kB
        Jason Rutherglen
      33. LUCENE-1314.patch
        36 kB
        Jason Rutherglen
      34. LUCENE-1314.patch
        34 kB
        Jason Rutherglen
      35. LUCENE-1314.patch
        18 kB
        Jason Rutherglen
      36. LUCENE-1314.patch
        17 kB
        Jason Rutherglen

        Activity

        Hide
        Jason Rutherglen added a comment -

        lucene-1314.patch

        First cut at this, seems to work as desired. Need to come up with a test case for it.

        Does anyone know how to turn off Eclipse automatically changing the import statements? I am not making it reformat but if I edit some code in a file it sees fit to reformat the imports.

        Show
        Jason Rutherglen added a comment - lucene-1314.patch First cut at this, seems to work as desired. Need to come up with a test case for it. Does anyone know how to turn off Eclipse automatically changing the import statements? I am not making it reformat but if I edit some code in a file it sees fit to reformat the imports.
        Hide
        Jason Rutherglen added a comment -

        A package protected field "boolean openNewFieldsReader = true;" (defaults to true to mimic previous behavior) should be added to SegmentReader to allow subclasses to determine if they want a new fieldsReader opened everytime a reopen occurs. The SegmentReader.doClose would need to not close fieldsReader if the openNewFieldsReader was set to false.

        The SegmentReader.reopenSegment method directly instantiates a SegmentReader rather than using IMPL like SegmentReader.get(Directory dir, SegmentInfo si, SegmentInfos sis, boolean closeDir, boolean ownDir, int readBufferSize, boolean doOpenStores) does.

        In my SegmentReader subclass I am passing a lock and passing a reference to fieldsReader for global locking and a single fieldsReader across all instances. Otherwise there are too many instances of fieldsReader and file descriptors will be used up.

        Show
        Jason Rutherglen added a comment - A package protected field "boolean openNewFieldsReader = true;" (defaults to true to mimic previous behavior) should be added to SegmentReader to allow subclasses to determine if they want a new fieldsReader opened everytime a reopen occurs. The SegmentReader.doClose would need to not close fieldsReader if the openNewFieldsReader was set to false. The SegmentReader.reopenSegment method directly instantiates a SegmentReader rather than using IMPL like SegmentReader.get(Directory dir, SegmentInfo si, SegmentInfos sis, boolean closeDir, boolean ownDir, int readBufferSize, boolean doOpenStores) does. In my SegmentReader subclass I am passing a lock and passing a reference to fieldsReader for global locking and a single fieldsReader across all instances. Otherwise there are too many instances of fieldsReader and file descriptors will be used up.
        Hide
        Jason Rutherglen added a comment -

        lucene-1314.patch

        Included the changes mentioned to allow a subclass of SegmentReader to not load fieldsReader on every reopen. Added the IMPL based instantiation in reopenSegment. SegmentReader.doClose does not close fieldsReader if openNewFieldsReader is false.

        Show
        Jason Rutherglen added a comment - lucene-1314.patch Included the changes mentioned to allow a subclass of SegmentReader to not load fieldsReader on every reopen. Added the IMPL based instantiation in reopenSegment. SegmentReader.doClose does not close fieldsReader if openNewFieldsReader is false.
        Hide
        Jason Rutherglen added a comment -

        lucene-1314.patch

        Added SegmentReader protected BitVector cloneDeletedDocs() that allows subclasses to do the cloning. This will allow for pooling reuse if many reopens occur and many BitVectors are created.

        Show
        Jason Rutherglen added a comment - lucene-1314.patch Added SegmentReader protected BitVector cloneDeletedDocs() that allows subclasses to do the cloning. This will allow for pooling reuse if many reopens occur and many BitVectors are created.
        Hide
        Nadav Har'El added a comment -

        At first glance, my opinion was that adding this flag to reopen() is confusing. reopen()'s current behavior is explained well in the documentation, and has a particular use case in mind (checking if the index has changed, and if it has, reopen it). Frankly, I didn't understand why reopen() should (even with the addition of a new parameter) clone or "copy on write" the IndexReader when the index hasn't changed.
        If this capability is needed, wouldn't it have been clearer if IndexReader had some new clone() or copyOnWrite() (in IndexReader's case, a write would actually be a delete...) method that can be called to get a new object that behaves independently from the previous one when it comes to writing (again, a delete)?
        In your code, you could then do something like

        newIndexReader = indexReader.reopen();
        if(newIndexReader==indexReader)
        newIndexReader = indexReader.clone(); // copy on write
        else

        { oldIndexReader.close(); // most applications won't do this here, but never mind now. }

        indexReader = newIndexReader;

        I thought that this was a cleaner API, because reopen() isn't complicated with an extra flag that has nothing to do with its intended function, and the new clone() or copyOnWrite() method can also be used in other situations when you want different objects of the same index to handle deletes separately.

        But on second glance, it dawned on me: You can't actually delete on both objects at once, because when you start deleting in one object, it holds a lock and then you can't do deletions in the second object! So I have to admit, the usefulness of of a general clone/copyOnWrite feature for IndexReader is quite limited. My suggestion above can still be the API, but I admit it will hardly be useful in any situation except (the rare situation nowadays of?) a reopen() and later deletes.

        Show
        Nadav Har'El added a comment - At first glance, my opinion was that adding this flag to reopen() is confusing. reopen()'s current behavior is explained well in the documentation, and has a particular use case in mind (checking if the index has changed, and if it has, reopen it). Frankly, I didn't understand why reopen() should (even with the addition of a new parameter) clone or "copy on write" the IndexReader when the index hasn't changed. If this capability is needed, wouldn't it have been clearer if IndexReader had some new clone() or copyOnWrite() (in IndexReader's case, a write would actually be a delete...) method that can be called to get a new object that behaves independently from the previous one when it comes to writing (again, a delete)? In your code, you could then do something like newIndexReader = indexReader.reopen(); if(newIndexReader==indexReader) newIndexReader = indexReader.clone(); // copy on write else { oldIndexReader.close(); // most applications won't do this here, but never mind now. } indexReader = newIndexReader; I thought that this was a cleaner API, because reopen() isn't complicated with an extra flag that has nothing to do with its intended function, and the new clone() or copyOnWrite() method can also be used in other situations when you want different objects of the same index to handle deletes separately. But on second glance, it dawned on me: You can't actually delete on both objects at once, because when you start deleting in one object, it holds a lock and then you can't do deletions in the second object! So I have to admit, the usefulness of of a general clone/copyOnWrite feature for IndexReader is quite limited. My suggestion above can still be the API, but I admit it will hardly be useful in any situation except (the rare situation nowadays of?) a reopen() and later deletes.
        Hide
        Michael McCandless added a comment -

        In my SegmentReader subclass I am passing a lock and passing a reference to fieldsReader for global locking and a single fieldsReader across all instances. Otherwise there are too many instances of fieldsReader and file descriptors will be used up.

        Maybe instead we should just fix access to FieldsReader to be thread safe, either by making FieldsReader itself thread safe, or by doing something similar to what's done for TermVectorsReader (where each thread makes a "shallow" clone of the original TermVectorsReader, held in a ThreadLocal instance). If we do that, then in SegmentReader.doReopen() we never have to clone FieldsReader.

        Show
        Michael McCandless added a comment - In my SegmentReader subclass I am passing a lock and passing a reference to fieldsReader for global locking and a single fieldsReader across all instances. Otherwise there are too many instances of fieldsReader and file descriptors will be used up. Maybe instead we should just fix access to FieldsReader to be thread safe, either by making FieldsReader itself thread safe, or by doing something similar to what's done for TermVectorsReader (where each thread makes a "shallow" clone of the original TermVectorsReader, held in a ThreadLocal instance). If we do that, then in SegmentReader.doReopen() we never have to clone FieldsReader.
        Hide
        Jason Rutherglen added a comment -

        Here is the code of the SegmentReader subclass. Using the clone terminology would work as well, inside of SegmentReader the clone would most likely reuse SegmentReader.reopenSegment. The subclass turns off locking by overriding acquireWriteLock and having it do nothing. I do not know a general fix for the locking issue mentioned "it holds a lock and then you can't do deletions in the second object". Perhaps there is a way using lock less commits. It is possible to have SegmentReader implement if deletes occur to an earlier IndexReader and a flush is tried it fails, rather than fail in a newer IndexReader like it would now. This would require keeping track of later IndexReaders which is something Ocean does outside of IndexReader.

        As far as the FieldsReader, given how many SegmentReaders Ocean creates (up to one per update), a shallow clone threadlocal would still potentially create many file descriptors. I would rather see a synchronized FieldsReader, or simply use the approach in the code below. The external lock used seems ok because there is little competition for reading Documents, no more than normal a Lucene application using a single IndexReader loading documents for N results.

        public class OceanSegmentReader extends SegmentReader {
          protected ReentrantLock fieldsReaderLock;
          
          public OceanSegmentReader() {
            openNewFieldsReader = false;
          }
          
          protected void doInitialize() {
            fieldsReaderLock = new ReentrantLock();
          }
          
          protected void acquireWriteLock() throws IOException {
          }
          
          protected synchronized DirectoryIndexReader doReopen(SegmentInfos infos, boolean force) throws CorruptIndexException, IOException {
            OceanSegmentReader segmentReader = (OceanSegmentReader)super.doReopen(infos, force);
            segmentReader.fieldsReaderLock = fieldsReaderLock;
            return segmentReader;
          }
          
          /**
           * @throws CorruptIndexException
           *           if the index is corrupt
           * @throws IOException
           *           if there is a low-level IO error
           */
          public synchronized Document document(int n, FieldSelector fieldSelector) throws CorruptIndexException, IOException {
            ensureOpen();
            if (isDeleted(n))
              throw new IllegalArgumentException("attempt to access a deleted document");
            fieldsReaderLock.lock();
            try {
              return getFieldsReader().doc(n, fieldSelector);
            } finally {
              fieldsReaderLock.unlock();
            }
          }
        }
        
        Show
        Jason Rutherglen added a comment - Here is the code of the SegmentReader subclass. Using the clone terminology would work as well, inside of SegmentReader the clone would most likely reuse SegmentReader.reopenSegment. The subclass turns off locking by overriding acquireWriteLock and having it do nothing. I do not know a general fix for the locking issue mentioned "it holds a lock and then you can't do deletions in the second object". Perhaps there is a way using lock less commits. It is possible to have SegmentReader implement if deletes occur to an earlier IndexReader and a flush is tried it fails, rather than fail in a newer IndexReader like it would now. This would require keeping track of later IndexReaders which is something Ocean does outside of IndexReader. As far as the FieldsReader, given how many SegmentReaders Ocean creates (up to one per update), a shallow clone threadlocal would still potentially create many file descriptors. I would rather see a synchronized FieldsReader, or simply use the approach in the code below. The external lock used seems ok because there is little competition for reading Documents, no more than normal a Lucene application using a single IndexReader loading documents for N results. public class OceanSegmentReader extends SegmentReader { protected ReentrantLock fieldsReaderLock; public OceanSegmentReader() { openNewFieldsReader = false ; } protected void doInitialize() { fieldsReaderLock = new ReentrantLock(); } protected void acquireWriteLock() throws IOException { } protected synchronized DirectoryIndexReader doReopen(SegmentInfos infos, boolean force) throws CorruptIndexException, IOException { OceanSegmentReader segmentReader = (OceanSegmentReader) super .doReopen(infos, force); segmentReader.fieldsReaderLock = fieldsReaderLock; return segmentReader; } /** * @ throws CorruptIndexException * if the index is corrupt * @ throws IOException * if there is a low-level IO error */ public synchronized Document document( int n, FieldSelector fieldSelector) throws CorruptIndexException, IOException { ensureOpen(); if (isDeleted(n)) throw new IllegalArgumentException( "attempt to access a deleted document" ); fieldsReaderLock.lock(); try { return getFieldsReader().doc(n, fieldSelector); } finally { fieldsReaderLock.unlock(); } } }
        Hide
        Jason Rutherglen added a comment -

        Using the patch and the above subclass of SegmentReader received the following bug. I am assuming it has something to do with SegmentInfos committing. Ideally the new clone method of IndexReader will avoid things like reloading SegmentInfos from disk each time. That will probably slow down the rapid updates too much.

        1) testSearch(org.apache.lucene.ocean.TestSearch)java.lang.AssertionError: delete count mismatch: info=1 vs BitVector=5
        at org.apache.lucene.index.SegmentReader.loadDeletedDocs(SegmentReader.java:365)
        at org.apache.lucene.index.SegmentReader.initialize(SegmentReader.java:328)
        at org.apache.lucene.index.SegmentReader.get(SegmentReader.java:267)
        at org.apache.lucene.index.SegmentReader.get(SegmentReader.java:235)
        at org.apache.lucene.index.DirectoryIndexReader$1.doBody(DirectoryIndexReader.java:90)
        at org.apache.lucene.index.SegmentInfos$FindSegmentsFile.run(SegmentInfos.java:649)
        at org.apache.lucene.index.DirectoryIndexReader.open(DirectoryIndexReader.java:97)
        at org.apache.lucene.index.IndexReader.open(IndexReader.java:213)
        at org.apache.lucene.index.IndexReader.open(IndexReader.java:209)
        
        Show
        Jason Rutherglen added a comment - Using the patch and the above subclass of SegmentReader received the following bug. I am assuming it has something to do with SegmentInfos committing. Ideally the new clone method of IndexReader will avoid things like reloading SegmentInfos from disk each time. That will probably slow down the rapid updates too much. 1) testSearch(org.apache.lucene.ocean.TestSearch)java.lang.AssertionError: delete count mismatch: info=1 vs BitVector=5 at org.apache.lucene.index.SegmentReader.loadDeletedDocs(SegmentReader.java:365) at org.apache.lucene.index.SegmentReader.initialize(SegmentReader.java:328) at org.apache.lucene.index.SegmentReader.get(SegmentReader.java:267) at org.apache.lucene.index.SegmentReader.get(SegmentReader.java:235) at org.apache.lucene.index.DirectoryIndexReader$1.doBody(DirectoryIndexReader.java:90) at org.apache.lucene.index.SegmentInfos$FindSegmentsFile.run(SegmentInfos.java:649) at org.apache.lucene.index.DirectoryIndexReader.open(DirectoryIndexReader.java:97) at org.apache.lucene.index.IndexReader.open(IndexReader.java:213) at org.apache.lucene.index.IndexReader.open(IndexReader.java:209)
        Hide
        Michael McCandless added a comment -

        Using the patch and the above subclass of SegmentReader received the following bug. I am assuming it has something to do with SegmentInfos committing. Ideally the new clone method of IndexReader will avoid things like reloading SegmentInfos from disk each time. That will probably slow down the rapid updates too much.

        Right, that exception happens because you are carrying your own deletedDocs in memory to the new SegmentReader without first saving them to the _X_N.del file for that segment. The new clone() approach definitely should not reload the segments_N file, and thus not call SegmentReader.initialize.

        Show
        Michael McCandless added a comment - Using the patch and the above subclass of SegmentReader received the following bug. I am assuming it has something to do with SegmentInfos committing. Ideally the new clone method of IndexReader will avoid things like reloading SegmentInfos from disk each time. That will probably slow down the rapid updates too much. Right, that exception happens because you are carrying your own deletedDocs in memory to the new SegmentReader without first saving them to the _X_N.del file for that segment. The new clone() approach definitely should not reload the segments_N file, and thus not call SegmentReader.initialize.
        Hide
        Michael McCandless added a comment -

        It is possible to have SegmentReader implement if deletes occur to an earlier IndexReader and a flush is tried it fails, rather than fail in a newer IndexReader like it would now. This would require keeping track of later IndexReaders which is something Ocean does outside of IndexReader.

        I think this is tricky, since SegmentReader doesn't explicitly track whether there is a "cloned" reader out there. As things stand now, there is no such thing as a cloned reader, and so the only way that another SegmentReader is out there is if there have been commits to the index, in which case isCurrent() returns false and the old reader will not allow deletes to be performed. I suppose we could look at the refCount of the IndexReader: any reader that has been cloned and not yet closed will have a refCount > 1, whereas the last reader returned from a clone() call will have refCount 1.

        So unless we try to track this, when there are N clones out there, any one of them will be allowed to grab the write lock when a change (deletion or setNorm) is attempted, thus preventing all the other clones (and all readers open on previous commits) from making changes.

        Show
        Michael McCandless added a comment - It is possible to have SegmentReader implement if deletes occur to an earlier IndexReader and a flush is tried it fails, rather than fail in a newer IndexReader like it would now. This would require keeping track of later IndexReaders which is something Ocean does outside of IndexReader. I think this is tricky, since SegmentReader doesn't explicitly track whether there is a "cloned" reader out there. As things stand now, there is no such thing as a cloned reader, and so the only way that another SegmentReader is out there is if there have been commits to the index, in which case isCurrent() returns false and the old reader will not allow deletes to be performed. I suppose we could look at the refCount of the IndexReader: any reader that has been cloned and not yet closed will have a refCount > 1, whereas the last reader returned from a clone() call will have refCount 1. So unless we try to track this, when there are N clones out there, any one of them will be allowed to grab the write lock when a change (deletion or setNorm) is attempted, thus preventing all the other clones (and all readers open on previous commits) from making changes.
        Hide
        Jason Rutherglen added a comment -

        lucene-1314.patch

        Added fieldsReaderLocal to SegmentReader. reopenSegment passed the fieldsReaderLocal and termVectorsLocal to the new SegmentReader. In the current version, a new termVectorsLocal is created for each SegmentReader which made reuse of the previously created TermVectorsReaders for a thread unavailable. The same is implemented for fieldsReaderLocal.

        The locking needs a default mechanism. For my own purposes I will likely turn it off, the bug I posted was really the fault of the Ocean code since fixed.

        Would like to be able to optionally have this line run in DirectoryIndexReader in reopen. Does it need to be run on a clone?

        SegmentInfos infos = new SegmentInfos();
        infos.read(directory, segmentFileName);

        Show
        Jason Rutherglen added a comment - lucene-1314.patch Added fieldsReaderLocal to SegmentReader. reopenSegment passed the fieldsReaderLocal and termVectorsLocal to the new SegmentReader. In the current version, a new termVectorsLocal is created for each SegmentReader which made reuse of the previously created TermVectorsReaders for a thread unavailable. The same is implemented for fieldsReaderLocal. The locking needs a default mechanism. For my own purposes I will likely turn it off, the bug I posted was really the fault of the Ocean code since fixed. Would like to be able to optionally have this line run in DirectoryIndexReader in reopen. Does it need to be run on a clone? SegmentInfos infos = new SegmentInfos(); infos.read(directory, segmentFileName);
        Hide
        Hoss Man added a comment -

        I haven't really been following this issue, but this comment caught my eye...

        So unless we try to track this, when there are N clones out there, any one of them will be allowed to grab the write lock when a change (deletion or setNorm) is attempted, thus preventing all the other clones (and all readers open on previous commits) from making changes.

        ...and gave me an erie sense of deja vu. looking back at LUCENE-743, the original approach for reopen was based on cloning and prompted this comment from me...

        https://issues.apache.org/jira/browse/LUCENE-743?focusedCommentId=12534123#action_12534123

        ...which led to some interesting discussion about what the semantics of cloning an IndexReader should be (even though ultimately cloning wasn't used).

        Show
        Hoss Man added a comment - I haven't really been following this issue, but this comment caught my eye... So unless we try to track this, when there are N clones out there, any one of them will be allowed to grab the write lock when a change (deletion or setNorm) is attempted, thus preventing all the other clones (and all readers open on previous commits) from making changes. ...and gave me an erie sense of deja vu. looking back at LUCENE-743 , the original approach for reopen was based on cloning and prompted this comment from me... https://issues.apache.org/jira/browse/LUCENE-743?focusedCommentId=12534123#action_12534123 ...which led to some interesting discussion about what the semantics of cloning an IndexReader should be (even though ultimately cloning wasn't used).
        Hide
        Michael McCandless added a comment -

        ...which led to some interesting discussion about what the semantics of cloning an IndexReader should be (even though ultimately cloning wasn't used).

        In fact that 2nd point you raised ("what happens when you clone an IndexReader that has pending changes") makes me nervous here. I think I'd prefer that we disallow that (throw an exception when this is attempted). Ie you can only clone a reader that has no pending changes.

        Also Jason you added set/getWriteLock: how come you couldn't just customize LockFactory for that, instead? Eg if you want to turn off locking you can just use NoLockFactory.

        Would like to be able to optionally have this line run in DirectoryIndexReader in reopen. Does it need to be run on a clone?

        SegmentInfos infos = new SegmentInfos();
        infos.read(directory, segmentFileName);

        I agree that line should not run on clone().

        Show
        Michael McCandless added a comment - ...which led to some interesting discussion about what the semantics of cloning an IndexReader should be (even though ultimately cloning wasn't used). In fact that 2nd point you raised ("what happens when you clone an IndexReader that has pending changes") makes me nervous here. I think I'd prefer that we disallow that (throw an exception when this is attempted). Ie you can only clone a reader that has no pending changes. Also Jason you added set/getWriteLock: how come you couldn't just customize LockFactory for that, instead? Eg if you want to turn off locking you can just use NoLockFactory. Would like to be able to optionally have this line run in DirectoryIndexReader in reopen. Does it need to be run on a clone? SegmentInfos infos = new SegmentInfos(); infos.read(directory, segmentFileName); I agree that line should not run on clone().
        Hide
        Jason Rutherglen added a comment -

        lucene-1314.patch

        SegmentReader.doReopen throws an IOException when deletedDocsDirty or normsDirty is true. DirectoryIndexReader getLock and setLock removed. Added isCurrent check in DirectoryIndexReader.doCommit, as per a few comments ago regarding making sure the commit only happens to the latest index.

        Show
        Jason Rutherglen added a comment - lucene-1314.patch SegmentReader.doReopen throws an IOException when deletedDocsDirty or normsDirty is true. DirectoryIndexReader getLock and setLock removed. Added isCurrent check in DirectoryIndexReader.doCommit, as per a few comments ago regarding making sure the commit only happens to the latest index.
        Hide
        Michael McCandless added a comment -

        I don't think we need to check isCurrent() in doCommit() because since
        the reader holds the write lock it is necessarily current?

        I can't compile with your patch. It seems like you have changes to
        BitVector.java which did't make it into the patch? Eg the getBits()
        method.

        I attached a new version with these changes:

        • Throw IllegalStateException if you try to clone a reader that has
          pending changes.
        • Clone the SegmentInfos, instead of just taking a reference, in
          DirectoryIndexReader
        • Changed "public abstract Object clone()" in IndexReader to be a
          method that throws UnsupportedOperationException instead
        • Renamed a few things; removed some whitespace only diffs.
        • Factored up doReopenOrClone into IndexReader

        One difference between clone() and reopen() is you force the
        deletedDocs BitVector to be cloned in SegmentReader during clone(),
        but not during reopen(). With reopen() we "declared" that if you make
        changes to your reopened reader, it's undefined what happens to your
        old readers. Ie, it's a "don't do that" situation.

        But with clone() the situation is reversed: the whole reason why you
        make a clone() is to isolate any changes in the new reader from being
        visible to the old reader. Given that, I think you also must clone()
        the norms right?

        Jason could you add cloning of norms, and add some unit tests, to the
        patch? Thanks.

        Show
        Michael McCandless added a comment - I don't think we need to check isCurrent() in doCommit() because since the reader holds the write lock it is necessarily current? I can't compile with your patch. It seems like you have changes to BitVector.java which did't make it into the patch? Eg the getBits() method. I attached a new version with these changes: Throw IllegalStateException if you try to clone a reader that has pending changes. Clone the SegmentInfos, instead of just taking a reference, in DirectoryIndexReader Changed "public abstract Object clone()" in IndexReader to be a method that throws UnsupportedOperationException instead Renamed a few things; removed some whitespace only diffs. Factored up doReopenOrClone into IndexReader One difference between clone() and reopen() is you force the deletedDocs BitVector to be cloned in SegmentReader during clone(), but not during reopen(). With reopen() we "declared" that if you make changes to your reopened reader, it's undefined what happens to your old readers. Ie, it's a "don't do that" situation. But with clone() the situation is reversed: the whole reason why you make a clone() is to isolate any changes in the new reader from being visible to the old reader. Given that, I think you also must clone() the norms right? Jason could you add cloning of norms, and add some unit tests, to the patch? Thanks.
        Hide
        Jason Rutherglen added a comment -

        > check isCurrent()

        I thought we wanted to check a commit on a clone that the index is current? Does it need to be in a clone only portion of the code? Which class is best?

        > clone() the norms

        We need to clone norms. I want to make cloning deletedDocs and norms optional mainly because it is a waste in Ocean to clone norms. Is the best way to give the option parameters to the clone method (breaking Cloneable)? An additional option could be readOnly. Perhaps norms or deletedDocs becomes readOnly if they are ref copied and not cloned. IndexReader.open and reopen would need a readOnly parameter. Or should a subclass of SegmentReader handle cloning or refing norms and deletedDocs.

        I think it may be easiest to have readOnly be a part of this patch. I wanted to separate out the FieldsReader synchronization code into a separate patch but then this patch would have been messed up without it (the new FieldsReader per SegmentReader issue). Readonly may end up being similar.

        The newlines is another Eclipse thing I haven't figured out yet.

        Show
        Jason Rutherglen added a comment - > check isCurrent() I thought we wanted to check a commit on a clone that the index is current? Does it need to be in a clone only portion of the code? Which class is best? > clone() the norms We need to clone norms. I want to make cloning deletedDocs and norms optional mainly because it is a waste in Ocean to clone norms. Is the best way to give the option parameters to the clone method (breaking Cloneable)? An additional option could be readOnly. Perhaps norms or deletedDocs becomes readOnly if they are ref copied and not cloned. IndexReader.open and reopen would need a readOnly parameter. Or should a subclass of SegmentReader handle cloning or refing norms and deletedDocs. I think it may be easiest to have readOnly be a part of this patch. I wanted to separate out the FieldsReader synchronization code into a separate patch but then this patch would have been messed up without it (the new FieldsReader per SegmentReader issue). Readonly may end up being similar. The newlines is another Eclipse thing I haven't figured out yet.
        Hide
        Michael McCandless added a comment -

        I thought we wanted to check a commit on a clone that the index is current? Does it need to be in a clone only portion of the code? Which class is best?

        I think checking when a change is first made (and grabbing the write.lock to prevent others) is safer? Else you could lose changes you had made. Ie if there are clones out there, the first one that starts making changes prevents any others from doing so. But I feel like I'm missing something about Ocean: was there some driver for checking on commit instead?

        We need to clone norms. I want to make cloning deletedDocs and norms optional mainly because it is a waste in Ocean to clone norms. Is the best way to give the option parameters to the clone method (breaking Cloneable)? An additional option could be readOnly. Perhaps norms or deletedDocs becomes readOnly if they are ref copied and not cloned. IndexReader.open and reopen would need a readOnly parameter. Or should a subclass of SegmentReader handle cloning or refing norms and deletedDocs.

        It'd be nice if we could do a copy-on-write approach. This way no copy is made when you first clone, but if you go to make a change, it makes a private copy only at that point. And you don't have to separately specify a readOnly up front only to find later you didn't pass in the right value.

        I think it may be easiest to have readOnly be a part of this patch. I wanted to separate out the FieldsReader synchronization code into a separate patch but then this patch would have been messed up without it (the new FieldsReader per SegmentReader issue). Readonly may end up being similar.

        Maybe instead we wait on the readOnly patch until we resolve this one (ie stage them)?

        Show
        Michael McCandless added a comment - I thought we wanted to check a commit on a clone that the index is current? Does it need to be in a clone only portion of the code? Which class is best? I think checking when a change is first made (and grabbing the write.lock to prevent others) is safer? Else you could lose changes you had made. Ie if there are clones out there, the first one that starts making changes prevents any others from doing so. But I feel like I'm missing something about Ocean: was there some driver for checking on commit instead? We need to clone norms. I want to make cloning deletedDocs and norms optional mainly because it is a waste in Ocean to clone norms. Is the best way to give the option parameters to the clone method (breaking Cloneable)? An additional option could be readOnly. Perhaps norms or deletedDocs becomes readOnly if they are ref copied and not cloned. IndexReader.open and reopen would need a readOnly parameter. Or should a subclass of SegmentReader handle cloning or refing norms and deletedDocs. It'd be nice if we could do a copy-on-write approach. This way no copy is made when you first clone, but if you go to make a change, it makes a private copy only at that point. And you don't have to separately specify a readOnly up front only to find later you didn't pass in the right value. I think it may be easiest to have readOnly be a part of this patch. I wanted to separate out the FieldsReader synchronization code into a separate patch but then this patch would have been messed up without it (the new FieldsReader per SegmentReader issue). Readonly may end up being similar. Maybe instead we wait on the readOnly patch until we resolve this one (ie stage them)?
        Hide
        Jason Rutherglen added a comment -

        There are really only two options here and perhaps this API will work.

        // true makes a copy of the data structure always, while false passes the reference if the data structure is read only or makes a copy if it is writeable.
        IndexReader.getCopy(boolean normsWriteable, boolean deletesWriteable)
        IndexReader.getCopyReadOnly() // defaults to getCopy(false, false)

        Clone can be removed or default to getCopy(true, true). The current APIs default to getCopy(true, true). It is good to make this explicit here so that the deletedDocs or norms cannot be changed later when it is the clear intention of the code. It is no different than RandomAccessFile(file, "r") and RandomAccessFile(file, "rw")

        Lucene is supposed to be designed for fast reads at the expense of writes, no? This code in SegmentReader with the deletedDocs and norms synchronization goes against that. I think it is important to figure out a solution to give users the option of removing synchronization in SegmentReader, users who are willing to give up a little bit in memory (norms or deletedDocs don't use very much anyways).

        > copy-on-write approach

        Is the problem with isDeleted now. The java.util.concurrent.CopyOnWriteArrayList for example uses a volatile list and synchronized update methods. Which will not work because of JDK1.4.

        > driver for checking on commit

        Yes, it is more of an assertion, it can be performed in Ocean as well.

        Show
        Jason Rutherglen added a comment - There are really only two options here and perhaps this API will work. // true makes a copy of the data structure always, while false passes the reference if the data structure is read only or makes a copy if it is writeable. IndexReader.getCopy(boolean normsWriteable, boolean deletesWriteable) IndexReader.getCopyReadOnly() // defaults to getCopy(false, false) Clone can be removed or default to getCopy(true, true). The current APIs default to getCopy(true, true). It is good to make this explicit here so that the deletedDocs or norms cannot be changed later when it is the clear intention of the code. It is no different than RandomAccessFile(file, "r") and RandomAccessFile(file, "rw") Lucene is supposed to be designed for fast reads at the expense of writes, no? This code in SegmentReader with the deletedDocs and norms synchronization goes against that. I think it is important to figure out a solution to give users the option of removing synchronization in SegmentReader, users who are willing to give up a little bit in memory (norms or deletedDocs don't use very much anyways). > copy-on-write approach Is the problem with isDeleted now. The java.util.concurrent.CopyOnWriteArrayList for example uses a volatile list and synchronized update methods. Which will not work because of JDK1.4. > driver for checking on commit Yes, it is more of an assertion, it can be performed in Ocean as well.
        Hide
        robert engels added a comment -

        volatile should work on just about all 1.4.1+ JVMs, which I think is the required JVM level for Lucene anyway...

        Show
        robert engels added a comment - volatile should work on just about all 1.4.1+ JVMs, which I think is the required JVM level for Lucene anyway...
        Hide
        Jason Rutherglen added a comment -

        Following up on the API comment, there can be a version of the norms or deletedDocs wrapper class for pre JDK1.5 that uses a synchronized accessor as demonstrated http://gee.cs.oswego.edu/dl/classes/EDU/oswego/cs/dl/util/concurrent/CopyOnWriteArrayList.java (works on JDK1.2 and above) and a version for JDK1.5 that uses volatile. This is only for the writeable norms or deletedDocs anyways, but will yield results for users who continue to use the default API with a writeable IndexReader. The null check can be synchronized and there can be a global setting that tells the IndexReader to instantiate a new deletedDocs or norms on init.

        Show
        Jason Rutherglen added a comment - Following up on the API comment, there can be a version of the norms or deletedDocs wrapper class for pre JDK1.5 that uses a synchronized accessor as demonstrated http://gee.cs.oswego.edu/dl/classes/EDU/oswego/cs/dl/util/concurrent/CopyOnWriteArrayList.java (works on JDK1.2 and above) and a version for JDK1.5 that uses volatile. This is only for the writeable norms or deletedDocs anyways, but will yield results for users who continue to use the default API with a writeable IndexReader. The null check can be synchronized and there can be a global setting that tells the IndexReader to instantiate a new deletedDocs or norms on init.
        Hide
        Michael McCandless added a comment -

        Why would you ever need to make a read-only clone of a writable IndexReader? In fact, once we have readOnly IndexReaders, why would you ever clone one? The [precarious] use case that set us down this path (adding clone()) in the first place was to make a clone so that you could make changes to the clone without affecting the original reader.

        I think clone() should just clone() and not "alter" the readOnly-ness of the original IndexReader? We could still then under the hood do copy-on-write. We can just do this ourselves – keep a boolean isShared in SegmentReader that's true when more than one SegmentReader is referencing the norms/deletedDocs.

        I do think we should make IndexReader.open take a readOnly boolean (LUCENE-1030). In fact, maybe we should go off do that one, first, since it may change our approach to clone? (Ie swap the order of these two)?

        Show
        Michael McCandless added a comment - Why would you ever need to make a read-only clone of a writable IndexReader? In fact, once we have readOnly IndexReaders, why would you ever clone one? The [precarious] use case that set us down this path (adding clone()) in the first place was to make a clone so that you could make changes to the clone without affecting the original reader. I think clone() should just clone() and not "alter" the readOnly-ness of the original IndexReader? We could still then under the hood do copy-on-write. We can just do this ourselves – keep a boolean isShared in SegmentReader that's true when more than one SegmentReader is referencing the norms/deletedDocs. I do think we should make IndexReader.open take a readOnly boolean ( LUCENE-1030 ). In fact, maybe we should go off do that one, first, since it may change our approach to clone? (Ie swap the order of these two)?
        Hide
        Jason Rutherglen added a comment -

        lucene-1314.patch

        Implemented copy on write for norms and deletedDocs when a cloned SegmentReader is created. A copy on write ref count is used to insure the correct number of copies are created.

        Includes a test case.

        Show
        Jason Rutherglen added a comment - lucene-1314.patch Implemented copy on write for norms and deletedDocs when a cloned SegmentReader is created. A copy on write ref count is used to insure the correct number of copies are created. Includes a test case.
        Hide
        Michael McCandless added a comment -

        Jason, I had some problems with the latest patch. First, it wouldn't compile, because createIndex was private in TestIndexReaderReopen.

        Once I changed that to protected, I'm seeing this failure in TestStressIndexing2:

        [junit] Testsuite: org.apache.lucene.index.TestStressIndexing2
        [junit] Tests run: 2, Failures: 2, Errors: 0, Time elapsed: 9.596 sec

        [junit] ------------- Standard Output ---------------
        [junit] [stored/uncompressed,indexed,termVector,termVectorOffsets,termVectorPosition<f71:??? >, stored/uncompressed,indexed,tokenized,termVector,termVectorOffsets,termVectorPosition<f99:E J H J H F E C I C >, stored/uncompressed,indexed,omitNorms<id:1000085>]
        [junit] [stored/uncompressed,indexed,termVector,termVectorOffsets,termVectorPosition<f27:???@?p+???l? >, stored/uncompressed,indexed,omitNorms<id:1000000>]
        [junit] [stored/uncompressed,indexed,omitNorms<id:12>]
        [junit] [stored/uncompressed,indexed,termVector,termVectorOffsets,termVectorPosition,omitNorms<f64:C >, stored/uncompressed,indexed,termVector,termVectorOffsets,termVectorPosition,omitNorms<f90:1???t?? >, stored/uncompressed,indexed,omitNorms<id:0>]
        [junit] ------------- ---------------- ---------------
        [junit] Testcase: testRandom(org.apache.lucene.index.TestStressIndexing2): FAILED
        [junit] expected:<3> but was:<2>
        [junit] junit.framework.AssertionFailedError: expected:<3> but was:<2>
        [junit] at org.apache.lucene.index.TestStressIndexing2.verifyEquals(TestStressIndexing2.java:336)
        [junit] at org.apache.lucene.index.TestStressIndexing2.verifyEquals(TestStressIndexing2.java:234)
        [junit] at org.apache.lucene.index.TestStressIndexing2.verifyEquals(TestStressIndexing2.java:193)
        [junit] at org.apache.lucene.index.TestStressIndexing2.testRandom(TestStressIndexing2.java:68)

        [junit] Testcase: testMultiConfig(org.apache.lucene.index.TestStressIndexing2): FAILED
        [junit] expected:<1> but was:<3>
        [junit] junit.framework.AssertionFailedError: expected:<1> but was:<3>
        [junit] at org.apache.lucene.index.TestStressIndexing2.verifyEquals(TestStressIndexing2.java:336)
        [junit] at org.apache.lucene.index.TestStressIndexing2.verifyEquals(TestStressIndexing2.java:234)
        [junit] at org.apache.lucene.index.TestStressIndexing2.verifyEquals(TestStressIndexing2.java:193)
        [junit] at org.apache.lucene.index.TestStressIndexing2.testMultiConfig(TestStressIndexing2.java:97)

        Are you seeing this too?

        Show
        Michael McCandless added a comment - Jason, I had some problems with the latest patch. First, it wouldn't compile, because createIndex was private in TestIndexReaderReopen. Once I changed that to protected, I'm seeing this failure in TestStressIndexing2: [junit] Testsuite: org.apache.lucene.index.TestStressIndexing2 [junit] Tests run: 2, Failures: 2, Errors: 0, Time elapsed: 9.596 sec [junit] ------------- Standard Output --------------- [junit] [stored/uncompressed,indexed,termVector,termVectorOffsets,termVectorPosition<f71:??? >, stored/uncompressed,indexed,tokenized,termVector,termVectorOffsets,termVectorPosition<f99:E J H J H F E C I C >, stored/uncompressed,indexed,omitNorms<id:1000085>] [junit] [stored/uncompressed,indexed,termVector,termVectorOffsets,termVectorPosition<f27:???@?p+???l? >, stored/uncompressed,indexed,omitNorms<id:1000000>] [junit] [stored/uncompressed,indexed,omitNorms<id:12>] [junit] [stored/uncompressed,indexed,termVector,termVectorOffsets,termVectorPosition,omitNorms<f64:C >, stored/uncompressed,indexed,termVector,termVectorOffsets,termVectorPosition,omitNorms<f90:1???t?? >, stored/uncompressed,indexed,omitNorms<id:0>] [junit] ------------- ---------------- --------------- [junit] Testcase: testRandom(org.apache.lucene.index.TestStressIndexing2): FAILED [junit] expected:<3> but was:<2> [junit] junit.framework.AssertionFailedError: expected:<3> but was:<2> [junit] at org.apache.lucene.index.TestStressIndexing2.verifyEquals(TestStressIndexing2.java:336) [junit] at org.apache.lucene.index.TestStressIndexing2.verifyEquals(TestStressIndexing2.java:234) [junit] at org.apache.lucene.index.TestStressIndexing2.verifyEquals(TestStressIndexing2.java:193) [junit] at org.apache.lucene.index.TestStressIndexing2.testRandom(TestStressIndexing2.java:68) [junit] Testcase: testMultiConfig(org.apache.lucene.index.TestStressIndexing2): FAILED [junit] expected:<1> but was:<3> [junit] junit.framework.AssertionFailedError: expected:<1> but was:<3> [junit] at org.apache.lucene.index.TestStressIndexing2.verifyEquals(TestStressIndexing2.java:336) [junit] at org.apache.lucene.index.TestStressIndexing2.verifyEquals(TestStressIndexing2.java:234) [junit] at org.apache.lucene.index.TestStressIndexing2.verifyEquals(TestStressIndexing2.java:193) [junit] at org.apache.lucene.index.TestStressIndexing2.testMultiConfig(TestStressIndexing2.java:97) Are you seeing this too?
        Hide
        Jason Rutherglen added a comment -

        I am seeing the error. It is not norms because the test does not test norms. It is either a problem with fieldsreader or deleted docs.

        Show
        Jason Rutherglen added a comment - I am seeing the error. It is not norms because the test does not test norms. It is either a problem with fieldsreader or deleted docs.
        Hide
        Jason Rutherglen added a comment -

        lucene-1314.patch

        docStoreOffset is now cloned in FieldsReader.clone() fixing the bug encountered by TestStressIndexing2.

        Show
        Jason Rutherglen added a comment - lucene-1314.patch docStoreOffset is now cloned in FieldsReader.clone() fixing the bug encountered by TestStressIndexing2.
        Hide
        Jason Rutherglen added a comment -

        lucene-1314.patch

        Same as previous, with TestIndexReaderClone

        Show
        Jason Rutherglen added a comment - lucene-1314.patch Same as previous, with TestIndexReaderClone
        Hide
        Jason Rutherglen added a comment -

        lucene-1314.patch

        Added protected DirectoryIndexReader.allowCloneWithChanges which allows clones with changes to be made. Added protected IndexReader.decRef(boolean flush) and protected IndexReader.close(boolean flush) which allows subclasses to optionally flush changes.

        Show
        Jason Rutherglen added a comment - lucene-1314.patch Added protected DirectoryIndexReader.allowCloneWithChanges which allows clones with changes to be made. Added protected IndexReader.decRef(boolean flush) and protected IndexReader.close(boolean flush) which allows subclasses to optionally flush changes.
        Hide
        Michael McCandless added a comment -

        Added protected DirectoryIndexReader.allowCloneWithChanges which allows clones with changes to be made.

        This makes me a bit nervous. What does it "mean" to clone an IndexReader that has changes? Normally Lucene only allows one writer at a time on the index.

        Show
        Michael McCandless added a comment - Added protected DirectoryIndexReader.allowCloneWithChanges which allows clones with changes to be made. This makes me a bit nervous. What does it "mean" to clone an IndexReader that has changes? Normally Lucene only allows one writer at a time on the index.
        Hide
        Jason Rutherglen added a comment -

        Because Ocean does not flush to disk with every transaction (deleteDocument then clone the reader) there is a need to allow cloning of readers that have not flushed changes. Rather than create a workaround, which I did that involves overriding clone and setting hasChanges=false, cloning and then setting hasChanges=true again, this seemed to be cleaner.

        Show
        Jason Rutherglen added a comment - Because Ocean does not flush to disk with every transaction (deleteDocument then clone the reader) there is a need to allow cloning of readers that have not flushed changes. Rather than create a workaround, which I did that involves overriding clone and setting hasChanges=false, cloning and then setting hasChanges=true again, this seemed to be cleaner.
        Hide
        Michael McCandless added a comment -

        Because Ocean does not flush to disk with every transaction (deleteDocument then clone the reader) there is a need to allow cloning of readers that have not flushed changes.

        But when you clone a reader with pending changes, is the old reader's write lock revoked? Meaning it is not allowed to commit (yet, continues to hold the changes it has)? And the newly cloned reader gets the write lock? Are further changes allowed against the old reader, or maybe it should become "frozen"?

        Show
        Michael McCandless added a comment - Because Ocean does not flush to disk with every transaction (deleteDocument then clone the reader) there is a need to allow cloning of readers that have not flushed changes. But when you clone a reader with pending changes, is the old reader's write lock revoked? Meaning it is not allowed to commit (yet, continues to hold the changes it has)? And the newly cloned reader gets the write lock? Are further changes allowed against the old reader, or maybe it should become "frozen"?
        Hide
        Jason Rutherglen added a comment -

        lucene-1314.patch

        Fixed bug in SegmentReader.reopenSegment that caused the ref count of deleted docs and norms to reset after the second clone. Added test in TestIndexReaderClone to test for this.

        Answering the question from a few comments ago, I don't think this patch needs to wait on IndexReader becoming read only.

        Show
        Jason Rutherglen added a comment - lucene-1314.patch Fixed bug in SegmentReader.reopenSegment that caused the ref count of deleted docs and norms to reset after the second clone. Added test in TestIndexReaderClone to test for this. Answering the question from a few comments ago, I don't think this patch needs to wait on IndexReader becoming read only.
        Hide
        Michael McCandless added a comment -

        Jason I'd like to bring closure on this one; I think it's a good
        addition.

        But the fact that this patch implies it's possible to clone an
        IndexReader that has pending changes makes me nervous.

        So, could you change the patch to not include the
        "allowCloneWithChanges" addition to IndexReader? And, instead factor
        out the current logic in DirectoryIndexReader.reopen that skips
        checking when there are pending changes or isCurrent() returns true
        into a new method "allowReopen()"? This way you can subclass and put
        Ocean's logic (allowing a reader with pending changes to be cloned)
        into allowReopen().

        This way Lucene itself does not allow cloning a reader that has
        changes. But subclasses (Ocean) can still do so.

        Show
        Michael McCandless added a comment - Jason I'd like to bring closure on this one; I think it's a good addition. But the fact that this patch implies it's possible to clone an IndexReader that has pending changes makes me nervous. So, could you change the patch to not include the "allowCloneWithChanges" addition to IndexReader? And, instead factor out the current logic in DirectoryIndexReader.reopen that skips checking when there are pending changes or isCurrent() returns true into a new method "allowReopen()"? This way you can subclass and put Ocean's logic (allowing a reader with pending changes to be cloned) into allowReopen(). This way Lucene itself does not allow cloning a reader that has changes. But subclasses (Ocean) can still do so.
        Hide
        Jason Rutherglen added a comment -

        Looks like something changed such that the test case no longer works for the norms cloning. Having some problems figuring out what changed as it used to work.

        Show
        Jason Rutherglen added a comment - Looks like something changed such that the test case no longer works for the norms cloning. Having some problems figuring out what changed as it used to work.
        Hide
        Jason Rutherglen added a comment -

        lucene-1314.patch

        The TestIndexReaderClone sometimes fails on the norms byte comparison. The TestIndexReaderReopen fails on the ref counting. I am not sure why yet but will look into it.

        Show
        Jason Rutherglen added a comment - lucene-1314.patch The TestIndexReaderClone sometimes fails on the norms byte comparison. The TestIndexReaderReopen fails on the ref counting. I am not sure why yet but will look into it.
        Hide
        Michael McCandless added a comment -

        Jason I'm seeing many other tests failing (besides TestIndexReaderReopen). Are you seeing these too? EG:

            [junit] Testcase: testHangOnClose(org.apache.lucene.index.TestAddIndexesNoOptimize):	Caused an ERROR
            [junit] MockRAMDirectory: cannot close: there are still open files: {_0.fdx=13}
            [junit] java.lang.RuntimeException: MockRAMDirectory: cannot close: there are still open files: {_0.fdx=13}
            [junit] 	at org.apache.lucene.store.MockRAMDirectory.close(MockRAMDirectory.java:292)
            [junit] 	at org.apache.lucene.index.TestAddIndexesNoOptimize.testHangOnClose(TestAddIndexesNoOptimize.java:537)
        

        The failures all seem to be because *.fdx files are not being closed properly.

        Show
        Michael McCandless added a comment - Jason I'm seeing many other tests failing (besides TestIndexReaderReopen). Are you seeing these too? EG: [junit] Testcase: testHangOnClose(org.apache.lucene.index.TestAddIndexesNoOptimize): Caused an ERROR [junit] MockRAMDirectory: cannot close: there are still open files: {_0.fdx=13} [junit] java.lang.RuntimeException: MockRAMDirectory: cannot close: there are still open files: {_0.fdx=13} [junit] at org.apache.lucene.store.MockRAMDirectory.close(MockRAMDirectory.java:292) [junit] at org.apache.lucene.index.TestAddIndexesNoOptimize.testHangOnClose(TestAddIndexesNoOptimize.java:537) The failures all seem to be because *.fdx files are not being closed properly.
        Hide
        Jason Rutherglen added a comment -

        Hi Michael,

        It used to work this patch. It was not easy to implement the first time. I did a merge of the new code and these are the resulting problems. Do you know what has changed since 2.3.1? Or around July? I am surprised at some of the errors as they should be unrelated to how the patch works. I may need to go ahead and break up the patch into the FieldsReader synchronization optimization (or is this already in trunk?) in order to isolate the issues. What do you think?

        Jason

        Show
        Jason Rutherglen added a comment - Hi Michael, It used to work this patch. It was not easy to implement the first time. I did a merge of the new code and these are the resulting problems. Do you know what has changed since 2.3.1? Or around July? I am surprised at some of the errors as they should be unrelated to how the patch works. I may need to go ahead and break up the patch into the FieldsReader synchronization optimization (or is this already in trunk?) in order to isolate the issues. What do you think? Jason
        Hide
        Jason Rutherglen added a comment -

        Yes I am seeing the same error.

        Show
        Jason Rutherglen added a comment - Yes I am seeing the same error.
        Hide
        Michael McCandless added a comment -

        OK I think you are missing a "cloneableIndexStream.close()" in FieldsReader.close. For me that fixes all the tests failing with *.fdx "still open".

        But I still see 5 failures in TestIndexReaderReopen.

        I may need to go ahead and break up the patch into the FieldsReader synchronization optimization (or is this already in trunk?) in order to isolate the issues.

        I think that's a good idea.

        Show
        Michael McCandless added a comment - OK I think you are missing a "cloneableIndexStream.close()" in FieldsReader.close. For me that fixes all the tests failing with *.fdx "still open". But I still see 5 failures in TestIndexReaderReopen. I may need to go ahead and break up the patch into the FieldsReader synchronization optimization (or is this already in trunk?) in order to isolate the issues. I think that's a good idea.
        Hide
        Jason Rutherglen added a comment -

        The remaining design issue with this patch is the writeLock. It would seems best to share the writeLock and ref it across instances of clones SegmentReaders. It is hurting my brain because it would be best to simply have a shared context object that represents all shared data between SegmentReaders but this probably will not work as it would move variables such as deletedDocs, norms, and the writeLock.

        Show
        Jason Rutherglen added a comment - The remaining design issue with this patch is the writeLock. It would seems best to share the writeLock and ref it across instances of clones SegmentReaders. It is hurting my brain because it would be best to simply have a shared context object that represents all shared data between SegmentReaders but this probably will not work as it would move variables such as deletedDocs, norms, and the writeLock.
        Hide
        Jason Rutherglen added a comment -

        Seems like 2 solutions to clone and pending updates in SegmentReader:

        1) Flush any pending changes on clone
        2) Throw an exception when the original SegmentReader receives new updates (i.e. deleteDocument is called) and on close does not flush the changes. Flush throws an exception reading "This segmentreader has pending changes and has been cloned and so cannot accept or flush updates" The problem with this one is it could lead to unpredictable behavior in that the user may assume the pending changes are being flushed (on close) and are not.

        Perhaps both can be supported with IndexReader.clone(boolean autoFlush) where the autoFlush=true tells the clone method to flush pending updates (if there are any), and false means keep the pending changes but throw an exception on flush.

        Show
        Jason Rutherglen added a comment - Seems like 2 solutions to clone and pending updates in SegmentReader: 1) Flush any pending changes on clone 2) Throw an exception when the original SegmentReader receives new updates (i.e. deleteDocument is called) and on close does not flush the changes. Flush throws an exception reading "This segmentreader has pending changes and has been cloned and so cannot accept or flush updates" The problem with this one is it could lead to unpredictable behavior in that the user may assume the pending changes are being flushed (on close) and are not. Perhaps both can be supported with IndexReader.clone(boolean autoFlush) where the autoFlush=true tells the clone method to flush pending updates (if there are any), and false means keep the pending changes but throw an exception on flush.
        Hide
        Jason Rutherglen added a comment -

        Decided to simply release the lock in SegmentReader when a clone occurs. If the original segmentreader receives updates (such as deleteDocument) after being cloned then it throws a LockObtainFailedException.

        Show
        Jason Rutherglen added a comment - Decided to simply release the lock in SegmentReader when a clone occurs. If the original segmentreader receives updates (such as deleteDocument) after being cloned then it throws a LockObtainFailedException.
        Hide
        Jason Rutherglen added a comment -

        LUCENE-1314.patch

        • SegmentReader.reopenSegment accepts doClone variable
        • Ref class created for SegmentReader
        • TestIndexReaderClone focuses on testing SegmentReader
        • TestIndexReaderClone creates an index, clones and tests the deletedDocs reference counting
        • During a clone of a SegmentReader, the writeLock is released
        • Norms are not copy on write yet
        • TestIndexReaderReopen passes

        If the general method looks ok, norms cloning will be implemented

        Show
        Jason Rutherglen added a comment - LUCENE-1314 .patch SegmentReader.reopenSegment accepts doClone variable Ref class created for SegmentReader TestIndexReaderClone focuses on testing SegmentReader TestIndexReaderClone creates an index, clones and tests the deletedDocs reference counting During a clone of a SegmentReader, the writeLock is released Norms are not copy on write yet TestIndexReaderReopen passes If the general method looks ok, norms cloning will be implemented
        Hide
        Jason Rutherglen added a comment -

        LUCENE-1314.patch

        • bytes now cloning properly compared to previous patch
        Show
        Jason Rutherglen added a comment - LUCENE-1314 .patch bytes now cloning properly compared to previous patch
        Hide
        Jason Rutherglen added a comment -

        LUCENE-1314.patch

        • Added TestIndexReaderCloneNorms because cloning the norms is really hard to do implemented as copy on write. There seem to be many caveats such as whether or not the norms stream is still open? testNormsRefCounting fails with a corruptedIndexException which I'm investigating.

        I now remember implementing a copy of just the bytes, and only editting them in the Norm object to get around these issues. Basically on clone, new Norm objects and a Map is created but the byte array of the cloned norm is shared. On a write, the bytes are cloned. This gets around needing to deal with the reader norm reference counting used by reopen, though is this a good idea?

        I'll try that and see if it works. Otherwise, suggestions besides hard cloning the norms for each clone?

        Show
        Jason Rutherglen added a comment - LUCENE-1314 .patch Added TestIndexReaderCloneNorms because cloning the norms is really hard to do implemented as copy on write. There seem to be many caveats such as whether or not the norms stream is still open? testNormsRefCounting fails with a corruptedIndexException which I'm investigating. I now remember implementing a copy of just the bytes, and only editting them in the Norm object to get around these issues. Basically on clone, new Norm objects and a Map is created but the byte array of the cloned norm is shared. On a write, the bytes are cloned. This gets around needing to deal with the reader norm reference counting used by reopen, though is this a good idea? I'll try that and see if it works. Otherwise, suggestions besides hard cloning the norms for each clone?
        Hide
        Jason Rutherglen added a comment -

        LUCENE-1314.patch

        • Implemented copying the writeLock reference in DirectorySegmentReader.doReopen to the cloned reader, readonly is set to true, acquireWriteLock throws LockObtainFailedException when readOnly is true
        • Tests try to generate LockObtainFailedException on a cloned reader by trying to update

        TODO:

        • Probably needs more tests from TestIndexReaderReopen such as a multithreading one
        Show
        Jason Rutherglen added a comment - LUCENE-1314 .patch Implemented copying the writeLock reference in DirectorySegmentReader.doReopen to the cloned reader, readonly is set to true, acquireWriteLock throws LockObtainFailedException when readOnly is true Tests try to generate LockObtainFailedException on a cloned reader by trying to update TODO: Probably needs more tests from TestIndexReaderReopen such as a multithreading one
        Hide
        Jason Rutherglen added a comment -

        LUCENE-1314.patch

        • Added TestIndexReaderClone.testParallelReader and testMixedReaders
        • Test-core Lucene tests pass

        I request assistance in what type of code properly tests multi threading for cloning readers

        Show
        Jason Rutherglen added a comment - LUCENE-1314 .patch Added TestIndexReaderClone.testParallelReader and testMixedReaders Test-core Lucene tests pass I request assistance in what type of code properly tests multi threading for cloning readers
        Hide
        Michael McCandless added a comment -

        > I request assistance in what type of code properly tests multi threading for cloning readers

        Shouldn't cloning be synchronized, at least the step that transfers the write lock to the cloned reader?

        It should also be synchronized to avoid acquisition of the write lock while the clone is underway. Ie, only one of those can "win".

        The first time a reader with pending changes is cloned, it transfers the write lock to the clone. If that same reader gets cloned again, that's fine (but no write lock needs transferring again).

        BTW can you sync up your patch to the latest trunk? Or, let me know which revision your patch is based on? (I'm having trouble applying the patch).

        Show
        Michael McCandless added a comment - > I request assistance in what type of code properly tests multi threading for cloning readers Shouldn't cloning be synchronized, at least the step that transfers the write lock to the cloned reader? It should also be synchronized to avoid acquisition of the write lock while the clone is underway. Ie, only one of those can "win". The first time a reader with pending changes is cloned, it transfers the write lock to the clone. If that same reader gets cloned again, that's fine (but no write lock needs transferring again). BTW can you sync up your patch to the latest trunk? Or, let me know which revision your patch is based on? (I'm having trouble applying the patch).
        Hide
        Jason Rutherglen added a comment -

        LUCENE-1314.patch

        • Updated to work with trunk
        • Cleaned up commented code, removed unused variables
        • All tests pass

        Michael M.:
        "Shouldn't cloning be synchronized, at least the step that transfers the write lock to the cloned reader?"

        The clone method is synchronized, is there something else that should be synchronized on? I'm trying to figure out (if it's necessary) the right unit test for the synchronization.

        Show
        Jason Rutherglen added a comment - LUCENE-1314 .patch Updated to work with trunk Cleaned up commented code, removed unused variables All tests pass Michael M.: "Shouldn't cloning be synchronized, at least the step that transfers the write lock to the cloned reader?" The clone method is synchronized, is there something else that should be synchronized on? I'm trying to figure out (if it's necessary) the right unit test for the synchronization.
        Hide
        Michael McCandless added a comment -

        > The clone method is synchronized, is there something else that should be synchronized on?

        I think that's sufficient.

        Show
        Michael McCandless added a comment - > The clone method is synchronized, is there something else that should be synchronized on? I think that's sufficient.
        Hide
        Jason Rutherglen added a comment -

        In the case of a reader that is already cloned, and clone is called again, do we want to throw an exception (if so what kind)? Or perhaps clone should call acquireWriteLock to ensure only the reader with the lock may clone?

        Show
        Jason Rutherglen added a comment - In the case of a reader that is already cloned, and clone is called again, do we want to throw an exception (if so what kind)? Or perhaps clone should call acquireWriteLock to ensure only the reader with the lock may clone?
        Hide
        Jason Rutherglen added a comment -

        LUCENE-1314.patch

        • DirectoryIndexReader.doReopen acquires the write lock on a clone to ensure
          only the cloned reader may pass the write lock to the clone reader.
          This protects against the case where another reader may be trying
          to clone at the same time.
        • All tests pass
        Show
        Jason Rutherglen added a comment - LUCENE-1314 .patch DirectoryIndexReader.doReopen acquires the write lock on a clone to ensure only the cloned reader may pass the write lock to the clone reader. This protects against the case where another reader may be trying to clone at the same time. All tests pass
        Hide
        Michael McCandless added a comment -

        OK I reviewed the patch; some comments:

        • We have clone & reopen methods on *Reader (ParallelReader,
          MultiReader) that are not synchronized; shouldn't they be
          synchronized as well?
        • You are still re-loading segments_N when cloning, which is
          incorrect (in fact I had fixed this in my patch above but it got
          lost); you should just clone segmentInfos instead.
        • In SegmentReader.java we have "if (doClone) acquireWriteLock();",
          which isn't right? Ie, if this reader does not currently have the
          write lock (it has no "local mods") it should not acquire it? One
          should be allowed to clone a stale reader?
        • Have you done any tests to see the cost of the copy-on-write
          cloning of deleted docs BitVector & norms? The first new mod to
          the cloned reader pays that penalty. Marvin's "tombstone"
          deletions would bring this penalty to near zero, but it's a big
          change (and should certainly be decoupled from this!).
        • In IndexReader.clone()'s javadocs can you state that on cloning a
          reader with pending modifications, the original reader then
          becomes readOnly (in addition to passing the write lock to the
          cloned reader)?
        • Can you rename IndexReader.cloneBitVector --> cloneDeletedDocs?
        • When you clone the deleted docs (because refCount is > 1) you are
          first decRef'ing the old one and then making the clone... can you
          change that so the decRef is done last? I don't think any actual
          bug would result from the way it is now, but let's be defensive
          (don't decRef something until you really are done using it).
        • Class Ref does not need to delcare its private int refCount as
          volatile; you always access that member from a synchronized
          context.
        • In SegmentReader.clone, you incRef the deleteDocsCopyOnWriteRef,
          but it might be null, right? Oh I see, it's always init'd to a
          new Ref() but then you make a new Ref() again on the first
          delete. Can you make it null by default? (Seems like it should
          be null if deletedDocs is).
        • Can you change Ref() so that it init's its refCount to 1? This
          way new Ref() need not immediately call incRef, and a Ref() with
          refCount 0 is never allowed out into the wild, and you should then
          add an "assert refCount > 0" in incRef as well.
        • In SegmentReader.doClose() you are failing to call
          deletedDocsCopyOnWriteRef.decRef(), so you have a refCount leak.
          Can you create a unit test that 1) opens reader 1, 2) does deletes
          on reader 1, 3) clones reader 1 --> reader 2, 4) closes reader 1,
          5) deletes more docs with reader 1, and 6) asserts that the
          deletedDocs BitVector did not get cloned? First verify the test
          fails, then fix the bug...
        • How should unDeleteAll() work? EG it seems like you must decRef
          the Ref, then set it to null? Can you add some tests for the
          various permutations of this?
        • SegmentReader.Norm now has two refCounts, and I think both are
          necessary. One tracks refs to the Norm instance itself and the
          other tracks refs to the byte[]. Can you add some comments
          explaining the difference (because it's confusing at first blush)?
        • I think the answer to your question "// is it necessary to clone
          everything?" in SegmentReader.Norm.clone() is "yes"; can you
          make sure you are cloning everything and then remove that comment?
        • In SegmentReader.doClose() we are also failing to decRef Norm
          instances (I think this is a pre-existing bug) and the newly added
          Refs to the byte[]'s as well. Can you fix both of these?
        • In SegmentReader.Norm.cloneBytes() don't you need to create a new
          bytesRef as well, because you are making a private copy at that
          point? Oh I see, you do this up in SegmentReader.doSetNorm; can
          you move it (= making a new bytesRef) down into cloneBytes()?
          Also, do the decRef of the old Ref last, not first, just like the
          deletedDocs case.
        • I think you might have an off-by-one error in Norm cloning. A
          newly cloned norm seems to share its byte[] with the original put
          gets a bytesRef with refCount 1? (Should be refCount 2?).
        • Can you rename deletedDocsCopyOnWriteRef -> deletedDocsRef?
        • Can you add "new and experimental" caveats to the package-private
          APIs you've added (cloneNormBytes, cloneDeletedDocs)?
        Show
        Michael McCandless added a comment - OK I reviewed the patch; some comments: We have clone & reopen methods on *Reader (ParallelReader, MultiReader) that are not synchronized; shouldn't they be synchronized as well? You are still re-loading segments_N when cloning, which is incorrect (in fact I had fixed this in my patch above but it got lost); you should just clone segmentInfos instead. In SegmentReader.java we have "if (doClone) acquireWriteLock();", which isn't right? Ie, if this reader does not currently have the write lock (it has no "local mods") it should not acquire it? One should be allowed to clone a stale reader? Have you done any tests to see the cost of the copy-on-write cloning of deleted docs BitVector & norms? The first new mod to the cloned reader pays that penalty. Marvin's "tombstone" deletions would bring this penalty to near zero, but it's a big change (and should certainly be decoupled from this!). In IndexReader.clone()'s javadocs can you state that on cloning a reader with pending modifications, the original reader then becomes readOnly (in addition to passing the write lock to the cloned reader)? Can you rename IndexReader.cloneBitVector --> cloneDeletedDocs? When you clone the deleted docs (because refCount is > 1) you are first decRef'ing the old one and then making the clone... can you change that so the decRef is done last? I don't think any actual bug would result from the way it is now, but let's be defensive (don't decRef something until you really are done using it). Class Ref does not need to delcare its private int refCount as volatile; you always access that member from a synchronized context. In SegmentReader.clone, you incRef the deleteDocsCopyOnWriteRef, but it might be null, right? Oh I see, it's always init'd to a new Ref() but then you make a new Ref() again on the first delete. Can you make it null by default? (Seems like it should be null if deletedDocs is). Can you change Ref() so that it init's its refCount to 1? This way new Ref() need not immediately call incRef, and a Ref() with refCount 0 is never allowed out into the wild, and you should then add an "assert refCount > 0" in incRef as well. In SegmentReader.doClose() you are failing to call deletedDocsCopyOnWriteRef.decRef(), so you have a refCount leak. Can you create a unit test that 1) opens reader 1, 2) does deletes on reader 1, 3) clones reader 1 --> reader 2, 4) closes reader 1, 5) deletes more docs with reader 1, and 6) asserts that the deletedDocs BitVector did not get cloned? First verify the test fails, then fix the bug... How should unDeleteAll() work? EG it seems like you must decRef the Ref, then set it to null? Can you add some tests for the various permutations of this? SegmentReader.Norm now has two refCounts, and I think both are necessary. One tracks refs to the Norm instance itself and the other tracks refs to the byte[]. Can you add some comments explaining the difference (because it's confusing at first blush)? I think the answer to your question "// is it necessary to clone everything?" in SegmentReader.Norm.clone() is "yes"; can you make sure you are cloning everything and then remove that comment? In SegmentReader.doClose() we are also failing to decRef Norm instances (I think this is a pre-existing bug) and the newly added Refs to the byte[]'s as well. Can you fix both of these? In SegmentReader.Norm.cloneBytes() don't you need to create a new bytesRef as well, because you are making a private copy at that point? Oh I see, you do this up in SegmentReader.doSetNorm; can you move it (= making a new bytesRef) down into cloneBytes()? Also, do the decRef of the old Ref last, not first, just like the deletedDocs case. I think you might have an off-by-one error in Norm cloning. A newly cloned norm seems to share its byte[] with the original put gets a bytesRef with refCount 1? (Should be refCount 2?). Can you rename deletedDocsCopyOnWriteRef -> deletedDocsRef? Can you add "new and experimental" caveats to the package-private APIs you've added (cloneNormBytes, cloneDeletedDocs)?
        Hide
        Jason Rutherglen added a comment -

        LUCENE-1314.patch

        Michael, thanks for reviewing the patch in such detail. All of your comments have been included in the latest version of the patch.

        M.M: In SegmentReader.java we have "if (doClone) acquireWriteLock();", which isn't right? Ie, if this reader does not currently have the write lock (it has no "local mods") it should not acquire it? One should be allowed to clone a stale reader?

        Cloning a stale reader is fixed in the patch. The problem is the user may get into trouble by updating the stale reader which was debated before. I got the impression insuring the reader being updated was the latest was important.

        M.M.: Have you done any tests to see the cost of the copy-on-write cloning of deleted docs BitVector & norms? The first new mod to the cloned reader pays that penalty. Marvin's "tombstone" deletions would bring this penalty to near zero, but it's a big change (and should certainly be decoupled from this!).

        The cost of cloning them meaning the creating a new byte array or some other cost like memory consumption? I need to reread Marvin's tombstones which at first glance seemed to be an iterative approach to saving deletions that seems like a transaction log. Correct?

        M.M.: SegmentReader.Norm now has two refCounts, and I think both are necessary. One tracks refs to the Norm instance itself and the other tracks refs to the byte[]. Can you add some comments explaining the difference (because it's confusing at first blush)?

        Byte[] referencing is used because a new norm object needs to be created for each clone, and the byte array is all that is needed for sharing between cloned readers. The current norm referencing is for sharing between readers whereas the byte[] referencing is for copy on write which is independent of reader references.

        M.M.: In SegmentReader.doClose() you are failing to call deletedDocsCopyOnWriteRef.decRef(), so you have a refCount leak. Can you create a unit test that 1) opens reader 1, 2) does deletes on reader 1, 3) clones reader 1 --> reader 2, 4) closes reader 1, 5) deletes more docs with reader 1, and 6) asserts that the
        deletedDocs BitVector did not get cloned? First verify the test fails, then fix the bug...

        In regards to #5, the test cannot delete from reader 1 once it's closed. A method called TestIndexReaderClone.testSegmentReaderCloseReferencing was added to test this closing use case.

        Show
        Jason Rutherglen added a comment - LUCENE-1314 .patch Michael, thanks for reviewing the patch in such detail. All of your comments have been included in the latest version of the patch. M.M: In SegmentReader.java we have "if (doClone) acquireWriteLock();", which isn't right? Ie, if this reader does not currently have the write lock (it has no "local mods") it should not acquire it? One should be allowed to clone a stale reader? Cloning a stale reader is fixed in the patch. The problem is the user may get into trouble by updating the stale reader which was debated before. I got the impression insuring the reader being updated was the latest was important. M.M.: Have you done any tests to see the cost of the copy-on-write cloning of deleted docs BitVector & norms? The first new mod to the cloned reader pays that penalty. Marvin's "tombstone" deletions would bring this penalty to near zero, but it's a big change (and should certainly be decoupled from this!). The cost of cloning them meaning the creating a new byte array or some other cost like memory consumption? I need to reread Marvin's tombstones which at first glance seemed to be an iterative approach to saving deletions that seems like a transaction log. Correct? M.M.: SegmentReader.Norm now has two refCounts, and I think both are necessary. One tracks refs to the Norm instance itself and the other tracks refs to the byte[]. Can you add some comments explaining the difference (because it's confusing at first blush)? Byte[] referencing is used because a new norm object needs to be created for each clone, and the byte array is all that is needed for sharing between cloned readers. The current norm referencing is for sharing between readers whereas the byte[] referencing is for copy on write which is independent of reader references. M.M.: In SegmentReader.doClose() you are failing to call deletedDocsCopyOnWriteRef.decRef(), so you have a refCount leak. Can you create a unit test that 1) opens reader 1, 2) does deletes on reader 1, 3) clones reader 1 --> reader 2, 4) closes reader 1, 5) deletes more docs with reader 1, and 6) asserts that the deletedDocs BitVector did not get cloned? First verify the test fails, then fix the bug... In regards to #5, the test cannot delete from reader 1 once it's closed. A method called TestIndexReaderClone.testSegmentReaderCloseReferencing was added to test this closing use case.
        Hide
        Michael McCandless added a comment -

        > The problem is the user may get into trouble by updating the stale reader which was debated before. I got the impression insuring the reader being updated was the latest was important.

        But: when one attempts to change a stale reader, that's caught when
        trying to acquire the write lock? (Ie during clone I think you don't
        need to also check for this).

        > The cost of cloning them meaning the creating a new byte array

        Yeah I was thinking CPU cost of creating & copying deleted docs &
        norms; I was just curious (I don't think we have to measure this
        before committing).

        > I need to reread Marvin's tombstones which at first glance seemed to be an iterative approach to saving deletions that seems like a transaction log. Correct?

        Similar to a transaction log in that the size of what's written is in
        proportion to how many changes (deletions) you made. But different in
        that there is no other data structure (ie the tombstones are the
        representation of the deletes) and so the tombstones are used "live"
        (whereas transaction log is typically "played back" on next startup
        after a failure).

        If we had tombstones to represent deletes in Lucene then any new
        deletions would not require any cloning of prior deletions. Ie there
        would be no copy-on-write.

        > M.M.: SegmentReader.Norm now has two refCounts, and I think both are necessary. One tracks refs to the Norm instance itself and the other tracks refs to the byte[]. Can you add some comments explaining the difference (because it's confusing at first blush)?
        >
        > Byte[] referencing is used because a new norm object needs to be created for each clone, and the byte array is all that is needed for sharing between cloned readers. The current norm referencing is for sharing between readers whereas the byte[] referencing is for copy on write which is independent of reader references.

        Got it. Can you put this into the javadocs in the Norm class?

        > M.M.: In SegmentReader.doClose() you are failing to call deletedDocsCopyOnWriteRef.decRef(), so you have a refCount leak. Can you create a unit test that 1) opens reader 1, 2) does deletes on reader 1, 3) clones reader 1 --> reader 2, 4) closes reader 1, 5) deletes more docs with reader 1, and 6) asserts that the
        deletedDocs BitVector did not get cloned? First verify the test fails, then fix the bug...
        >
        > In regards to #5, the test cannot delete from reader 1 once it's closed. A method called TestIndexReaderClone.testSegmentReaderCloseReferencing was added to test this closing use case.

        Woops – I meant "5) deletes more docs with reader 2". Test case
        looks good! Thanks.

        A few more comments:

        • Can you update javadocs of IndexReader.reopen to remove the
          warning about not doing modification operations? With
          copy-on-write you are now free to do deletes against the reopened
          reader with no impact to the reader you had reopened/cloned.
        • What is SegmentReader.doDecRef for? It seems dead?
        • SegmentReader.doUndeleteAll has 4 space indent (should be 2)
        • We have this in SegmentReader.reopenSegment:
          if (deletedDocsRef == null) deletedDocsRef = new Ref();
          else deletedDocsRef.incRef();
          

          But I think if I clone a reader with no deletes, the clone then
          [incorrectly] has a deletedDocsRef set? Can you fix that code to
          keep the invariant that if deleteDocs is null, so is
          deletedDocsRef, and v/v? Can you sprinkle asserts to make sure
          that invariant always holds?

        • In SegmentReader.decRef we have "if (deletedDocsRef != null &&
          deletedDocsRef.refCount() > 1) deletedDocsRef.decRef();" – but,
          you should not have to check if deletedDocsRef.refCount() > 1?
          Does something break when you remove that? (In which case I think
          we have a refCount bug lurking...)
        • The norm cloning logic in SegmentReader.reopenSegment needs to be
          cleaned up... eg we first sweep through each Norm, incRef'ing it,
          and then make 2nd pass to do full clone. Really we should have if
          (doClone) up front and do a single pass? Also: I think we need
          that same logic to re-open the singleNormStream for the clone case
          as well.
          .
          Hmm, in the non-single-norm stream case I think we also must
          re-open the norm file, rather than clone it, in Norm.clone(). I
          think if you 1) open reader 1(do no searching w/ it), 2) clone it
          --> reader 2, 3) close reader 1, 4) try to do a search against a
          field that then needs to load norms, you'll hit an
          AlreadyClosedException, because you had a cloned IndexInput vs a
          newly reopened one? Can you add that test case?
        • Why was this needed:
          if (doClone && normsDirty) {
            normsUpToDate = false;
          }
          

          It seems like leaving normsUpToDate as true should have worked
          (all the Norm instances are cloned anyway?).

        • In SegmentReader.doUndeleteAll, can you conditionalize on whether
          deletedDocs != null? In general rather than having separate
          checks for deleteDocs != null and deletedDocsRef != null, I'd
          prefer to check only deletedDocs != null and add an assert that
          deleteDocsRef != null, with else clause having assert
          deletedDocsRef == null.
        Show
        Michael McCandless added a comment - > The problem is the user may get into trouble by updating the stale reader which was debated before. I got the impression insuring the reader being updated was the latest was important. But: when one attempts to change a stale reader, that's caught when trying to acquire the write lock? (Ie during clone I think you don't need to also check for this). > The cost of cloning them meaning the creating a new byte array Yeah I was thinking CPU cost of creating & copying deleted docs & norms; I was just curious (I don't think we have to measure this before committing). > I need to reread Marvin's tombstones which at first glance seemed to be an iterative approach to saving deletions that seems like a transaction log. Correct? Similar to a transaction log in that the size of what's written is in proportion to how many changes (deletions) you made. But different in that there is no other data structure (ie the tombstones are the representation of the deletes) and so the tombstones are used "live" (whereas transaction log is typically "played back" on next startup after a failure). If we had tombstones to represent deletes in Lucene then any new deletions would not require any cloning of prior deletions. Ie there would be no copy-on-write. > M.M.: SegmentReader.Norm now has two refCounts, and I think both are necessary. One tracks refs to the Norm instance itself and the other tracks refs to the byte[]. Can you add some comments explaining the difference (because it's confusing at first blush)? > > Byte[] referencing is used because a new norm object needs to be created for each clone, and the byte array is all that is needed for sharing between cloned readers. The current norm referencing is for sharing between readers whereas the byte[] referencing is for copy on write which is independent of reader references. Got it. Can you put this into the javadocs in the Norm class? > M.M.: In SegmentReader.doClose() you are failing to call deletedDocsCopyOnWriteRef.decRef(), so you have a refCount leak. Can you create a unit test that 1) opens reader 1, 2) does deletes on reader 1, 3) clones reader 1 --> reader 2, 4) closes reader 1, 5) deletes more docs with reader 1, and 6) asserts that the deletedDocs BitVector did not get cloned? First verify the test fails, then fix the bug... > > In regards to #5, the test cannot delete from reader 1 once it's closed. A method called TestIndexReaderClone.testSegmentReaderCloseReferencing was added to test this closing use case. Woops – I meant "5) deletes more docs with reader 2". Test case looks good! Thanks. A few more comments: Can you update javadocs of IndexReader.reopen to remove the warning about not doing modification operations? With copy-on-write you are now free to do deletes against the reopened reader with no impact to the reader you had reopened/cloned. What is SegmentReader.doDecRef for? It seems dead? SegmentReader.doUndeleteAll has 4 space indent (should be 2) We have this in SegmentReader.reopenSegment: if (deletedDocsRef == null ) deletedDocsRef = new Ref(); else deletedDocsRef.incRef(); But I think if I clone a reader with no deletes, the clone then [incorrectly] has a deletedDocsRef set? Can you fix that code to keep the invariant that if deleteDocs is null, so is deletedDocsRef, and v/v? Can you sprinkle asserts to make sure that invariant always holds? In SegmentReader.decRef we have "if (deletedDocsRef != null && deletedDocsRef.refCount() > 1) deletedDocsRef.decRef();" – but, you should not have to check if deletedDocsRef.refCount() > 1? Does something break when you remove that? (In which case I think we have a refCount bug lurking...) The norm cloning logic in SegmentReader.reopenSegment needs to be cleaned up... eg we first sweep through each Norm, incRef'ing it, and then make 2nd pass to do full clone. Really we should have if (doClone) up front and do a single pass? Also: I think we need that same logic to re-open the singleNormStream for the clone case as well. . Hmm, in the non-single-norm stream case I think we also must re-open the norm file, rather than clone it, in Norm.clone(). I think if you 1) open reader 1(do no searching w/ it), 2) clone it --> reader 2, 3) close reader 1, 4) try to do a search against a field that then needs to load norms, you'll hit an AlreadyClosedException, because you had a cloned IndexInput vs a newly reopened one? Can you add that test case? Why was this needed: if (doClone && normsDirty) { normsUpToDate = false ; } It seems like leaving normsUpToDate as true should have worked (all the Norm instances are cloned anyway?). In SegmentReader.doUndeleteAll, can you conditionalize on whether deletedDocs != null? In general rather than having separate checks for deleteDocs != null and deletedDocsRef != null, I'd prefer to check only deletedDocs != null and add an assert that deleteDocsRef != null, with else clause having assert deletedDocsRef == null.
        Hide
        Jason Rutherglen added a comment -

        Everything in the previous post should be working and completed. TestIndexReaderReopen.testThreadSafety is creating a bug in the deletedDocs referencing which is related to

        if (!success) {
        // An exception occured during reopen, we have to decRef the norms
        // that we incRef'ed already and close singleNormsStream and FieldsReader
        clone.decRef();
        }
        

        at the bottom of SegmentReader.reopenSegment. I am finished for the day and posted what is completed otherwise.

        > "Similar to a transaction log in that the size of what's written is in
        proportion to how many changes (deletions) you made. But different in
        that there is no other data structure (ie the tombstones are the
        representation of the deletes) and so the tombstones are used "live"
        (whereas transaction log is typically "played back" on next startup
        after a failure).

        If we had tombstones to represent deletes in Lucene then any new
        deletions would not require any cloning of prior deletions. Ie there
        would be no copy-on-write."

        Definitely interesting, how do tombstones work with BitVector?

        I changed Norm.clone to Norm.cloneNorm because it needs to throw an IOException, the clone interface does not allow exceptions and it's hidden inside of SegmentReader so the naming conventions should not matter.

        Show
        Jason Rutherglen added a comment - Everything in the previous post should be working and completed. TestIndexReaderReopen.testThreadSafety is creating a bug in the deletedDocs referencing which is related to if (!success) { // An exception occured during reopen, we have to decRef the norms // that we incRef'ed already and close singleNormsStream and FieldsReader clone.decRef(); } at the bottom of SegmentReader.reopenSegment. I am finished for the day and posted what is completed otherwise. > "Similar to a transaction log in that the size of what's written is in proportion to how many changes (deletions) you made. But different in that there is no other data structure (ie the tombstones are the representation of the deletes) and so the tombstones are used "live" (whereas transaction log is typically "played back" on next startup after a failure). If we had tombstones to represent deletes in Lucene then any new deletions would not require any cloning of prior deletions. Ie there would be no copy-on-write." Definitely interesting, how do tombstones work with BitVector? I changed Norm.clone to Norm.cloneNorm because it needs to throw an IOException, the clone interface does not allow exceptions and it's hidden inside of SegmentReader so the naming conventions should not matter.
        Hide
        Michael McCandless added a comment -

        > Definitely interesting, how do tombstones work with BitVector?

        They would replace BitVector with an iterator API (eg DocIDSet). SegmentTermDocs would then AND together the term's postings with the deleted docs postings. I mocked up a rough test of iteration vs random-access and tentatively found that iteration was a bit faster if %tg deletes was less than 10% or so, but then more costly if it was higher. I didn't dig much into it though.

        Once Lucene access deletes via iterator, then multiple tombstone streams could be merged during searching.

        I'll look at the patch. Thanks Jason!

        Show
        Michael McCandless added a comment - > Definitely interesting, how do tombstones work with BitVector? They would replace BitVector with an iterator API (eg DocIDSet). SegmentTermDocs would then AND together the term's postings with the deleted docs postings. I mocked up a rough test of iteration vs random-access and tentatively found that iteration was a bit faster if %tg deletes was less than 10% or so, but then more costly if it was higher. I didn't dig much into it though. Once Lucene access deletes via iterator, then multiple tombstone streams could be merged during searching. I'll look at the patch. Thanks Jason!
        Hide
        Jason Rutherglen added a comment -

        I executed on Eclipse Mac OS X on a 4 core box (core's significant due to the threads). I ran TestIndexReaderReopen.testThreadSafety 2 times in debug mode it worked, thought that debug mode wasn't making the bug reproduce so tried just running the test and it passed again. The 5th time it gave an error in debug mode. The test case fails consistently when SegmentReader.reopenSegment success == false and decRef is called afterwards in the finally clause. It seems that calling this decRef on the newly cloned object is causing the assertion error which is possibly related to threading. Probably because the decRef on the failed clone is decrementing one too many times on a deletedDocsRef used by another reader and causing the following assertion error. I'm not sure if this is a real bug or an issue that the test case should ignore.

        java.lang.AssertionError
        	at org.apache.lucene.index.SegmentReader$Ref.decRef(SegmentReader.java:104)
        	at org.apache.lucene.index.SegmentReader.decRef(SegmentReader.java:249)
        	at org.apache.lucene.index.MultiSegmentReader.doClose(MultiSegmentReader.java:413)
        	at org.apache.lucene.index.IndexReader.decRef(IndexReader.java:157)
        	at org.apache.lucene.index.IndexReader.close(IndexReader.java:990)
        	at org.apache.lucene.index.TestIndexReaderReopen$9.run(TestIndexReaderReopen.java:703)
        	at org.apache.lucene.index.TestIndexReaderReopen$ReaderThread.run(TestIndexReaderReopen.java:818)
        
        Show
        Jason Rutherglen added a comment - I executed on Eclipse Mac OS X on a 4 core box (core's significant due to the threads). I ran TestIndexReaderReopen.testThreadSafety 2 times in debug mode it worked, thought that debug mode wasn't making the bug reproduce so tried just running the test and it passed again. The 5th time it gave an error in debug mode. The test case fails consistently when SegmentReader.reopenSegment success == false and decRef is called afterwards in the finally clause. It seems that calling this decRef on the newly cloned object is causing the assertion error which is possibly related to threading. Probably because the decRef on the failed clone is decrementing one too many times on a deletedDocsRef used by another reader and causing the following assertion error. I'm not sure if this is a real bug or an issue that the test case should ignore. java.lang.AssertionError at org.apache.lucene.index.SegmentReader$Ref.decRef(SegmentReader.java:104) at org.apache.lucene.index.SegmentReader.decRef(SegmentReader.java:249) at org.apache.lucene.index.MultiSegmentReader.doClose(MultiSegmentReader.java:413) at org.apache.lucene.index.IndexReader.decRef(IndexReader.java:157) at org.apache.lucene.index.IndexReader.close(IndexReader.java:990) at org.apache.lucene.index.TestIndexReaderReopen$9.run(TestIndexReaderReopen.java:703) at org.apache.lucene.index.TestIndexReaderReopen$ReaderThread.run(TestIndexReaderReopen.java:818)
        Hide
        Jason Rutherglen added a comment -

        LUCENE-1314.patch

        All tests pass.

        IndexReader.close was made non-final to override in SegmentReader. This is due to the propagation of the method calls to SegmentReader.doClose previously passed through decRef which could be called by IndexReader.decRef or IndexReader.close. In order to decref the copy on write refs, the close method needs to decrement the references, rather than simply the decRef method. This caused the bug found in the previous comment where if decRef was called the deletedDocsRef did not need to also be decrefed which was the cause of the ref count assertion failing.

        Occasionally TestIndexReaderReopen.testThreadSafety fails due to an already closed exception. Trunk however also fails periodically. Given multi threading of reopen/close is usually unlikely I am not sure it is worth investigating further.

        Fixed norm byte refs not decrefing on close.

        Fixed cloneNorm() byteRef being created when there is no byte array, added assertion check.

        Show
        Jason Rutherglen added a comment - LUCENE-1314 .patch All tests pass. IndexReader.close was made non-final to override in SegmentReader. This is due to the propagation of the method calls to SegmentReader.doClose previously passed through decRef which could be called by IndexReader.decRef or IndexReader.close. In order to decref the copy on write refs, the close method needs to decrement the references, rather than simply the decRef method. This caused the bug found in the previous comment where if decRef was called the deletedDocsRef did not need to also be decrefed which was the cause of the ref count assertion failing. Occasionally TestIndexReaderReopen.testThreadSafety fails due to an already closed exception. Trunk however also fails periodically. Given multi threading of reopen/close is usually unlikely I am not sure it is worth investigating further. Fixed norm byte refs not decrefing on close. Fixed cloneNorm() byteRef being created when there is no byte array, added assertion check.
        Hide
        Michael McCandless added a comment -

        > Occasionally TestIndexReaderReopen.testThreadSafety fails due to an already closed exception. Trunk however also fails periodically.

        Jason, I don't see this. I'm running TestIndexReaderReopen via the command line (not through eclipse). Are you able to see a failure (on trunk) via the command line?

        Show
        Michael McCandless added a comment - > Occasionally TestIndexReaderReopen.testThreadSafety fails due to an already closed exception. Trunk however also fails periodically. Jason, I don't see this. I'm running TestIndexReaderReopen via the command line (not through eclipse). Are you able to see a failure (on trunk) via the command line?
        Hide
        Jason Rutherglen added a comment -

        I haven't seen this error via the command line, only sporadically in eclipse. Is there a way with ant to only test one test case?
        Tried:
        "ant -Dtestcase=org.apache.lucene.index.TestIndexReaderReopen test-core" which according to the Wiki http://wiki.apache.org/lucene-java/HowToContribute should work.

        Show
        Jason Rutherglen added a comment - I haven't seen this error via the command line, only sporadically in eclipse. Is there a way with ant to only test one test case? Tried: "ant -Dtestcase=org.apache.lucene.index.TestIndexReaderReopen test-core" which according to the Wiki http://wiki.apache.org/lucene-java/HowToContribute should work.
        Hide
        Erik Hatcher added a comment -

        Is there a way with ant to only test one test case?
        Tried:
        "ant -Dtestcase=org.apache.lucene.index.TestIndexReaderReopen test-core" which according to the Wiki http://wiki.apache.org/lucene-java/HowToContribute should work.

        The value of the testcase parameter fits in this way **/$

        {testcase}

        .java in common-build.xml, so in your case it'd be -Dtestcase=TestIndexReaderReopen

        Show
        Erik Hatcher added a comment - Is there a way with ant to only test one test case? Tried: "ant -Dtestcase=org.apache.lucene.index.TestIndexReaderReopen test-core" which according to the Wiki http://wiki.apache.org/lucene-java/HowToContribute should work. The value of the testcase parameter fits in this way **/$ {testcase} .java in common-build.xml, so in your case it'd be -Dtestcase=TestIndexReaderReopen
        Hide
        Jason Rutherglen added a comment -

        That worked Erik.

        I executed TestIndexReaderReopen using the LUCENE-1314 patch 8 times via command line and did not see the error.

        Then tried TestIndexReaderReopen in trunk and saw this the first time:

        common.test:
            [mkdir] Created dir: /Users/jrutherg/dev/lucenetrunk/trunk/build/test
            [junit] Testsuite: org.apache.lucene.index.TestIndexReaderReopen
            [junit] this IndexReader is closed)
            [junit] Tests run: 15, Failures: 1, Errors: 0, Time elapsed: 19.125 sec
            [junit] 
            [junit] ------------- Standard Output ---------------
            [junit] java.io.FileNotFoundException: _0_6.del
            [junit] 	at org.apache.lucene.store.RAMDirectory.openInput(RAMDirectory.java:237)
            [junit] 	at org.apache.lucene.util.BitVector.<init>(BitVector.java:235)
            [junit] 	at org.apache.lucene.index.SegmentReader.loadDeletedDocs(SegmentReader.java:412)
            [junit] 	at org.apache.lucene.index.SegmentReader.reopenSegment(SegmentReader.java:499)
            [junit] 	at org.apache.lucene.index.MultiSegmentReader.<init>(MultiSegmentReader.java:112)
            [junit] 	at org.apache.lucene.index.SegmentReader.doReopen(SegmentReader.java:442)
            [junit] 	at org.apache.lucene.index.DirectoryIndexReader$2.doBody(DirectoryIndexReader.java:153)
            [junit] 	at org.apache.lucene.index.SegmentInfos$FindSegmentsFile.run(SegmentInfos.java:688)
            [junit] 	at org.apache.lucene.index.DirectoryIndexReader.reopen(DirectoryIndexReader.java:175)
            [junit] 	at org.apache.lucene.index.TestIndexReaderReopen$9.run(TestIndexReaderReopen.java:685)
            [junit] 	at org.apache.lucene.index.TestIndexReaderReopen$ReaderThread.run(TestIndexReaderReopen.java:818)
            [junit] org.apache.lucene.store.AlreadyClosedException: this IndexReader is closed
            [junit] 	at org.apache.lucene.index.IndexReader.ensureOpen(IndexReader.java:196)
            [junit] 	at org.apache.lucene.index.SegmentReader.docFreq(SegmentReader.java:741)
            [junit] 	at org.apache.lucene.index.MultiSegmentReader.docFreq(MultiSegmentReader.java:378)
            [junit] 	at org.apache.lucene.search.IndexSearcher.docFreq(IndexSearcher.java:86)
            [junit] 	at org.apache.lucene.search.Similarity.idf(Similarity.java:481)
            [junit] 	at org.apache.lucene.search.TermQuery$TermWeight.<init>(TermQuery.java:44)
            [junit] 	at org.apache.lucene.search.TermQuery.createWeight(TermQuery.java:146)
            [junit] 	at org.apache.lucene.search.Query.weight(Query.java:95)
            [junit] 	at org.apache.lucene.search.Searcher.createWeight(Searcher.java:185)
            [junit] 	at org.apache.lucene.search.Searcher.search(Searcher.java:136)
            [junit] 	at org.apache.lucene.index.TestIndexReaderReopen$9.run(TestIndexReaderReopen.java:689)
            [junit] 	at org.apache.lucene.index.TestIndexReaderReopen$ReaderThread.run(TestIndexReaderReopen.java:818)
            [junit] ------------- ---------------- ---------------
            [junit] ------------- Standard Error -----------------
            [junit] java.io.FileNotFoundException: _0_6.del
            [junit] 	at org.apache.lucene.store.RAMDirectory.openInput(RAMDirectory.java:237)
            [junit] 	at org.apache.lucene.util.BitVector.<init>(BitVector.java:235)
            [junit] 	at org.apache.lucene.index.SegmentReader.loadDeletedDocs(SegmentReader.java:412)
            [junit] 	at org.apache.lucene.index.SegmentReader.reopenSegment(SegmentReader.java:499)
            [junit] 	at org.apache.lucene.index.MultiSegmentReader.<init>(MultiSegmentReader.java:112)
            [junit] 	at org.apache.lucene.index.SegmentReader.doReopen(SegmentReader.java:442)
            [junit] 	at org.apache.lucene.index.DirectoryIndexReader$2.doBody(DirectoryIndexReader.java:153)
            [junit] 	at org.apache.lucene.index.SegmentInfos$FindSegmentsFile.run(SegmentInfos.java:688)
            [junit] 	at org.apache.lucene.index.DirectoryIndexReader.reopen(DirectoryIndexReader.java:175)
            [junit] 	at org.apache.lucene.index.TestIndexReaderReopen$9.run(TestIndexReaderReopen.java:685)
            [junit] 	at org.apache.lucene.index.TestIndexReaderReopen$ReaderThread.run(TestIndexReaderReopen.java:818)
            [junit] ------------- ---------------- ---------------
            [junit] Testcase: testThreadSafety(org.apache.lucene.index.TestIndexReaderReopen):	FAILED
            [junit] Error occurred in thread Thread-51:
            [junit] this IndexReader is closed
            [junit] junit.framework.AssertionFailedError: Error occurred in thread Thread-51:
            [junit] this IndexReader is closed
            [junit] 	at org.apache.lucene.index.TestIndexReaderReopen.testThreadSafety(TestIndexReaderReopen.java:760)
            [junit] 
            [junit] 
            [junit] Test org.apache.lucene.index.TestIndexReaderReopen FAILED
        
        
        Show
        Jason Rutherglen added a comment - That worked Erik. I executed TestIndexReaderReopen using the LUCENE-1314 patch 8 times via command line and did not see the error. Then tried TestIndexReaderReopen in trunk and saw this the first time: common.test: [mkdir] Created dir: /Users/jrutherg/dev/lucenetrunk/trunk/build/test [junit] Testsuite: org.apache.lucene.index.TestIndexReaderReopen [junit] this IndexReader is closed) [junit] Tests run: 15, Failures: 1, Errors: 0, Time elapsed: 19.125 sec [junit] [junit] ------------- Standard Output --------------- [junit] java.io.FileNotFoundException: _0_6.del [junit] at org.apache.lucene.store.RAMDirectory.openInput(RAMDirectory.java:237) [junit] at org.apache.lucene.util.BitVector.<init>(BitVector.java:235) [junit] at org.apache.lucene.index.SegmentReader.loadDeletedDocs(SegmentReader.java:412) [junit] at org.apache.lucene.index.SegmentReader.reopenSegment(SegmentReader.java:499) [junit] at org.apache.lucene.index.MultiSegmentReader.<init>(MultiSegmentReader.java:112) [junit] at org.apache.lucene.index.SegmentReader.doReopen(SegmentReader.java:442) [junit] at org.apache.lucene.index.DirectoryIndexReader$2.doBody(DirectoryIndexReader.java:153) [junit] at org.apache.lucene.index.SegmentInfos$FindSegmentsFile.run(SegmentInfos.java:688) [junit] at org.apache.lucene.index.DirectoryIndexReader.reopen(DirectoryIndexReader.java:175) [junit] at org.apache.lucene.index.TestIndexReaderReopen$9.run(TestIndexReaderReopen.java:685) [junit] at org.apache.lucene.index.TestIndexReaderReopen$ReaderThread.run(TestIndexReaderReopen.java:818) [junit] org.apache.lucene.store.AlreadyClosedException: this IndexReader is closed [junit] at org.apache.lucene.index.IndexReader.ensureOpen(IndexReader.java:196) [junit] at org.apache.lucene.index.SegmentReader.docFreq(SegmentReader.java:741) [junit] at org.apache.lucene.index.MultiSegmentReader.docFreq(MultiSegmentReader.java:378) [junit] at org.apache.lucene.search.IndexSearcher.docFreq(IndexSearcher.java:86) [junit] at org.apache.lucene.search.Similarity.idf(Similarity.java:481) [junit] at org.apache.lucene.search.TermQuery$TermWeight.<init>(TermQuery.java:44) [junit] at org.apache.lucene.search.TermQuery.createWeight(TermQuery.java:146) [junit] at org.apache.lucene.search.Query.weight(Query.java:95) [junit] at org.apache.lucene.search.Searcher.createWeight(Searcher.java:185) [junit] at org.apache.lucene.search.Searcher.search(Searcher.java:136) [junit] at org.apache.lucene.index.TestIndexReaderReopen$9.run(TestIndexReaderReopen.java:689) [junit] at org.apache.lucene.index.TestIndexReaderReopen$ReaderThread.run(TestIndexReaderReopen.java:818) [junit] ------------- ---------------- --------------- [junit] ------------- Standard Error ----------------- [junit] java.io.FileNotFoundException: _0_6.del [junit] at org.apache.lucene.store.RAMDirectory.openInput(RAMDirectory.java:237) [junit] at org.apache.lucene.util.BitVector.<init>(BitVector.java:235) [junit] at org.apache.lucene.index.SegmentReader.loadDeletedDocs(SegmentReader.java:412) [junit] at org.apache.lucene.index.SegmentReader.reopenSegment(SegmentReader.java:499) [junit] at org.apache.lucene.index.MultiSegmentReader.<init>(MultiSegmentReader.java:112) [junit] at org.apache.lucene.index.SegmentReader.doReopen(SegmentReader.java:442) [junit] at org.apache.lucene.index.DirectoryIndexReader$2.doBody(DirectoryIndexReader.java:153) [junit] at org.apache.lucene.index.SegmentInfos$FindSegmentsFile.run(SegmentInfos.java:688) [junit] at org.apache.lucene.index.DirectoryIndexReader.reopen(DirectoryIndexReader.java:175) [junit] at org.apache.lucene.index.TestIndexReaderReopen$9.run(TestIndexReaderReopen.java:685) [junit] at org.apache.lucene.index.TestIndexReaderReopen$ReaderThread.run(TestIndexReaderReopen.java:818) [junit] ------------- ---------------- --------------- [junit] Testcase: testThreadSafety(org.apache.lucene.index.TestIndexReaderReopen): FAILED [junit] Error occurred in thread Thread -51: [junit] this IndexReader is closed [junit] junit.framework.AssertionFailedError: Error occurred in thread Thread -51: [junit] this IndexReader is closed [junit] at org.apache.lucene.index.TestIndexReaderReopen.testThreadSafety(TestIndexReaderReopen.java:760) [junit] [junit] [junit] Test org.apache.lucene.index.TestIndexReaderReopen FAILED
        Hide
        Michael McCandless added a comment -

        Odd, I still can't see it. Are you sure you have no local mods? Eg the line number in BitVector.java looks off (but others look OK).

        What's your full env? I'm also running on quad core Mac Pro, 10.5.5, with this java:

        Java(TM) SE Runtime Environment (build 1.6.0_07-b06-153)
        Java HotSpot(TM) 64-Bit Server VM (build 1.6.0_07-b06-57, mixed mode)
        
        Show
        Michael McCandless added a comment - Odd, I still can't see it. Are you sure you have no local mods? Eg the line number in BitVector.java looks off (but others look OK). What's your full env? I'm also running on quad core Mac Pro, 10.5.5, with this java: Java(TM) SE Runtime Environment (build 1.6.0_07-b06-153) Java HotSpot(TM) 64-Bit Server VM (build 1.6.0_07-b06-57, mixed mode)
        Hide
        Jason Rutherglen added a comment -

        Software
        System Version: Mac OS X 10.5.6 (9G55)
        Kernel Version: Darwin 9.6.0
        Boot Volume: Macintosh HD
        Hardware
        Model Name: Mac Pro
        Model Identifier: MacPro3,1
        Processor Name: Quad-Core Intel Xeon
        Processor Speed: 2.8 GHz
        Number Of Processors: 2
        Total Number Of Cores: 8
        L2 Cache (per processor): 12 MB
        Memory: 12 GB
        Bus Speed: 1.6 GHz
        Boot ROM Version: MP31.006C.B05
        SMC Version: 1.25f4
        Serial Number: G88151P4XYL

        java -version

        java version "1.5.0_16"
        Java(TM) 2 Runtime Environment, Standard Edition (build 1.5.0_16-b06-284)
        Java HotSpot(TM) Client VM (build 1.5.0_16-133, mixed mode, sharing)
        

        Does ant execute with -server turned on? My VM isn't 64bit?

        Show
        Jason Rutherglen added a comment - Software System Version: Mac OS X 10.5.6 (9G55) Kernel Version: Darwin 9.6.0 Boot Volume: Macintosh HD Hardware Model Name: Mac Pro Model Identifier: MacPro3,1 Processor Name: Quad-Core Intel Xeon Processor Speed: 2.8 GHz Number Of Processors: 2 Total Number Of Cores: 8 L2 Cache (per processor): 12 MB Memory: 12 GB Bus Speed: 1.6 GHz Boot ROM Version: MP31.006C.B05 SMC Version: 1.25f4 Serial Number: G88151P4XYL java -version java version "1.5.0_16" Java(TM) 2 Runtime Environment, Standard Edition (build 1.5.0_16-b06-284) Java HotSpot(TM) Client VM (build 1.5.0_16-133, mixed mode, sharing) Does ant execute with -server turned on? My VM isn't 64bit?
        Hide
        Michael McCandless added a comment -

        I started from the last patch and made some fixes to how norms are
        shared (new patch attached).

        I also fixed the reopen/clone unit tests to use MockRAMDirectory, to
        verify all open files are closed... which resulted in some interesting
        failures that are now fixed.

        With this change, we use fewer file descriptions during reopen &
        clone. Previously, the norms in the new reader opened a new
        IndexInput on reopen or clone; now we share the original one (similar
        to how "referencedSegmentReader" works). This is true for the "single
        norm stream" case too.

        It's now safe to make changes (norms, deletions) on a reopened reader
        (as well as cloned reader); it won't affect the previous reader (a
        private copy-on-write is made on the first change).

        All tests pass, except I still can't repro the failure Jason sees on
        trunk. I think we're getting close!

        Show
        Michael McCandless added a comment - I started from the last patch and made some fixes to how norms are shared (new patch attached). I also fixed the reopen/clone unit tests to use MockRAMDirectory, to verify all open files are closed... which resulted in some interesting failures that are now fixed. With this change, we use fewer file descriptions during reopen & clone. Previously, the norms in the new reader opened a new IndexInput on reopen or clone; now we share the original one (similar to how "referencedSegmentReader" works). This is true for the "single norm stream" case too. It's now safe to make changes (norms, deletions) on a reopened reader (as well as cloned reader); it won't affect the previous reader (a private copy-on-write is made on the first change). All tests pass, except I still can't repro the failure Jason sees on trunk. I think we're getting close!
        Hide
        Jason Rutherglen added a comment -

        Fixing the norms bytes loading is good, it seemed incorrect but I
        didn't want to mess with it as I didn't fully understand it.

        I executed TestIndexReaderReopen 7 times and did not see the error.

        Show
        Jason Rutherglen added a comment - Fixing the norms bytes loading is good, it seemed incorrect but I didn't want to mess with it as I didn't fully understand it. I executed TestIndexReaderReopen 7 times and did not see the error.
        Hide
        Jason Rutherglen added a comment -

        I think we should add reopen(boolean readonly) and clone(boolean
        readonly) as otherwise the only way to obtain a readonly reader is to
        call IR.open.

        Show
        Jason Rutherglen added a comment - I think we should add reopen(boolean readonly) and clone(boolean readonly) as otherwise the only way to obtain a readonly reader is to call IR.open.
        Hide
        Jason Rutherglen added a comment -

        This patch includes the previous patch. All tests pass.

        • IndexReader.clone(boolean openReadOnly) method added that returns a
          read only reader.
        • Previously on a clone, the DirectoryIndexReader.doReopen was
          looking up the latest segmentinfos file from the directory
          implementation. This was unnecessary because the current segmentinfos
          is being cloned. In this patch DirectoryIndexReader.clone uses the
          new doReopenSegmentInfos (which can also be used by LUCENE-1516).
          When clone returns a read only reader, the write lock etc is not
          shared.
        • SegmentReader. reopenSegment has a openReadOnly argument.
        • Added a test case in TestIndexReaderClone checking for read only
          readers being returned from the method clone openReadOnly=true.

        I decided against having reopen have a read only method because it
        does not always return a reader like clone does. If the existing
        reader is not read only and the returned reader is supposed to be
        read only, the current contract of the reopen method would break
        because if there are no changes it is supposed to return the existing
        reader. Also returning a read only reader is desired, clone may be
        used.

        Show
        Jason Rutherglen added a comment - This patch includes the previous patch. All tests pass. IndexReader.clone(boolean openReadOnly) method added that returns a read only reader. Previously on a clone, the DirectoryIndexReader.doReopen was looking up the latest segmentinfos file from the directory implementation. This was unnecessary because the current segmentinfos is being cloned. In this patch DirectoryIndexReader.clone uses the new doReopenSegmentInfos (which can also be used by LUCENE-1516 ). When clone returns a read only reader, the write lock etc is not shared. SegmentReader. reopenSegment has a openReadOnly argument. Added a test case in TestIndexReaderClone checking for read only readers being returned from the method clone openReadOnly=true. I decided against having reopen have a read only method because it does not always return a reader like clone does. If the existing reader is not read only and the returned reader is supposed to be read only, the current contract of the reopen method would break because if there are no changes it is supposed to return the existing reader. Also returning a read only reader is desired, clone may be used.
        Hide
        Michael McCandless added a comment -

        Jason I think TestIndexReaderClone.java is missing from your patch?

        Show
        Michael McCandless added a comment - Jason I think TestIndexReaderClone.java is missing from your patch?
        Hide
        Michael McCandless added a comment -

        > Previously on a clone, the DirectoryIndexReader.doReopen was
        > looking up the latest segmentinfos file from the directory
        > implementation. This was unnecessary because the current segmentinfos
        > is being cloned.

        Good! So now we don't even run the FindSegmentsFile at all during
        clone.

        > I think we should add reopen(boolean readonly) and clone(boolean
        > readonly) as otherwise the only way to obtain a readonly reader is to
        > call IR.open.

        I agree. It then seems like for realtime search you may want to clone
        a readOnly IR to do your searching (for better concurrency), and keep
        the writable IR for doing further changes.

        > IndexReader.clone(boolean openReadOnly) method added that returns a
        > read only reader.

        If I have a readOnly reader and I clone with readOnly=false, should
        that give me a non-readOnly clone? I think it should? But I think
        current patch doesn't handle that right? If so, could you add a test
        case & fix it?

        Then, if I have a non-readOnly reader and I clone it with
        readOnly=true, it seems like the semantics is to have the original
        reader keep the write lock, ie it's allowed to continue making changes
        if it wants, and the first change after that clone should
        copy-on-write? Can you add a test case verifying that?

        > I decided against having reopen have a read only method because it
        > does not always return a reader like clone does.

        OK.

        Show
        Michael McCandless added a comment - > Previously on a clone, the DirectoryIndexReader.doReopen was > looking up the latest segmentinfos file from the directory > implementation. This was unnecessary because the current segmentinfos > is being cloned. Good! So now we don't even run the FindSegmentsFile at all during clone. > I think we should add reopen(boolean readonly) and clone(boolean > readonly) as otherwise the only way to obtain a readonly reader is to > call IR.open. I agree. It then seems like for realtime search you may want to clone a readOnly IR to do your searching (for better concurrency), and keep the writable IR for doing further changes. > IndexReader.clone(boolean openReadOnly) method added that returns a > read only reader. If I have a readOnly reader and I clone with readOnly=false, should that give me a non-readOnly clone? I think it should? But I think current patch doesn't handle that right? If so, could you add a test case & fix it? Then, if I have a non-readOnly reader and I clone it with readOnly=true, it seems like the semantics is to have the original reader keep the write lock, ie it's allowed to continue making changes if it wants, and the first change after that clone should copy-on-write? Can you add a test case verifying that? > I decided against having reopen have a read only method because it > does not always return a reader like clone does. OK.
        Hide
        Jason Rutherglen added a comment -

        All tests pass

        Changed DirectoryIndexReader. acquireWriteLock to throw an
        UnsupportedOperationException (rather than a
        LockObtainFailedException) when the reader is readonly because
        TestIndexReader.testReadOnly was failing

        Added test for cloning a writeable reader to read only

        Cloning a read only reader into a writeable reader throws an
        exception. Added a test case

        Why does DirectoryIndexReader and SegmentReader have a readonly
        variable?

        Show
        Jason Rutherglen added a comment - All tests pass Changed DirectoryIndexReader. acquireWriteLock to throw an UnsupportedOperationException (rather than a LockObtainFailedException) when the reader is readonly because TestIndexReader.testReadOnly was failing Added test for cloning a writeable reader to read only Cloning a read only reader into a writeable reader throws an exception. Added a test case Why does DirectoryIndexReader and SegmentReader have a readonly variable?
        Hide
        Michael McCandless added a comment -

        Changed DirectoryIndexReader. acquireWriteLock to throw an
        UnsupportedOperationException (rather than a
        LockObtainFailedException) when the reader is readonly because
        TestIndexReader.testReadOnly was failing

        We can't really make that change – it's technically a change to back compat. Can we just fix the test that was failing (why was it failing?).

        Cloning a read only reader into a writeable reader throws an
        exception.

        Why not allow this? (It shouldn't be hard to allow it?)

        Why does DirectoryIndexReader and SegmentReader have a readonly
        variable?

        Hmm – that's no good. Can you remove SegmentReader's?

        Show
        Michael McCandless added a comment - Changed DirectoryIndexReader. acquireWriteLock to throw an UnsupportedOperationException (rather than a LockObtainFailedException) when the reader is readonly because TestIndexReader.testReadOnly was failing We can't really make that change – it's technically a change to back compat. Can we just fix the test that was failing (why was it failing?). Cloning a read only reader into a writeable reader throws an exception. Why not allow this? (It shouldn't be hard to allow it?) Why does DirectoryIndexReader and SegmentReader have a readonly variable? Hmm – that's no good. Can you remove SegmentReader's?
        Hide
        Jason Rutherglen added a comment -

        "Changed DirectoryIndexReader. acquireWriteLock to throw an
        UnsupportedOperationException (rather than a
        LockObtainFailedException) when the reader is readonly because
        TestIndexReader.testReadOnly was failing"

        We can't really make that change - it's technically a change to back
        compat. Can we just fix the test that was failing (why was it
        failing?).

        It's been added in this patch so there isn't a back compat issue. I'm
        not sure why it wasn't being thrown in previous testing.
        acquireWriteLock in ReadOnly*Reader throws
        UnsupportedOperationException making the patch compatible with
        existing behavior.

        "Cloning a read only reader into a writeable reader throws an
        exception."

        Why not allow this? (It shouldn't be hard to allow it?)

        While it seem logical, it's not allowed because the read only reader
        doesn't hold the write lock which it needs to pass on to the
        writeable reader.

        SegmentReader.readOnly variable removed

        All tests pass

        Show
        Jason Rutherglen added a comment - "Changed DirectoryIndexReader. acquireWriteLock to throw an UnsupportedOperationException (rather than a LockObtainFailedException) when the reader is readonly because TestIndexReader.testReadOnly was failing" We can't really make that change - it's technically a change to back compat. Can we just fix the test that was failing (why was it failing?). It's been added in this patch so there isn't a back compat issue. I'm not sure why it wasn't being thrown in previous testing. acquireWriteLock in ReadOnly*Reader throws UnsupportedOperationException making the patch compatible with existing behavior. "Cloning a read only reader into a writeable reader throws an exception." Why not allow this? (It shouldn't be hard to allow it?) While it seem logical, it's not allowed because the read only reader doesn't hold the write lock which it needs to pass on to the writeable reader. SegmentReader.readOnly variable removed All tests pass
        Hide
        Michael McCandless added a comment -

        It's been added in this patch so there isn't a back compat issue.

        OK, I was confused. This new exception is thrown if you 1) have a
        non-readOnly reader1, 2) reopen it with readOnly=false (thus marking
        reader1 as readOnly=true), and 3) try to make a change with the
        now-readOnly reader.

        OK I like throwing UOE. It's the same as what you get if you open
        a readOnly reader and try to make a change.

        While it seem logical, it's not allowed because the read only reader
        doesn't hold the write lock which it needs to pass on to the
        writeable reader.

        But that's OK? Ie, cloning to a non-readOnly reader when it doesn't
        already have the write lock simply means the new reader is allowed to
        attempt acquiring the write lock (even though it doesn't already have
        the write lock).

        Ie "being non-readOnly" and "holding the write lock" are two separate
        things.

        I can see this being useful if you are an app that doens't often need
        to make changes w/ the reader... so you hold a readOnly reader most of
        the time, but then when you need to make a change you clone it to
        non-readOnly clone to make changes.

        I also wonder what should happen if you 1) have a non-readOnly
        reader1, but 2) it has no changes pending (does not hold the write
        lock) and 3) you clone it to a non-readOnly clone reader2. I think
        reader1 should not be marked readOnly this case, because it has no
        pending changes. Ie I think at this point reader1 & reader2 should be
        interchangeable?

        Show
        Michael McCandless added a comment - It's been added in this patch so there isn't a back compat issue. OK, I was confused. This new exception is thrown if you 1) have a non-readOnly reader1, 2) reopen it with readOnly=false (thus marking reader1 as readOnly=true), and 3) try to make a change with the now-readOnly reader. OK I like throwing UOE. It's the same as what you get if you open a readOnly reader and try to make a change. While it seem logical, it's not allowed because the read only reader doesn't hold the write lock which it needs to pass on to the writeable reader. But that's OK? Ie, cloning to a non-readOnly reader when it doesn't already have the write lock simply means the new reader is allowed to attempt acquiring the write lock (even though it doesn't already have the write lock). Ie "being non-readOnly" and "holding the write lock" are two separate things. I can see this being useful if you are an app that doens't often need to make changes w/ the reader... so you hold a readOnly reader most of the time, but then when you need to make a change you clone it to non-readOnly clone to make changes. I also wonder what should happen if you 1) have a non-readOnly reader1, but 2) it has no changes pending (does not hold the write lock) and 3) you clone it to a non-readOnly clone reader2. I think reader1 should not be marked readOnly this case, because it has no pending changes. Ie I think at this point reader1 & reader2 should be interchangeable?
        Hide
        Jason Rutherglen added a comment -

        Added TestIndexReaderClone.testCloneNoChangesStilReadOnly, changed
        behavior such that if there are no changes, the original reader is
        not set to readonly (the previous behavior was to set readonly to
        true even if the reader did not have changes).

        Show
        Jason Rutherglen added a comment - Added TestIndexReaderClone.testCloneNoChangesStilReadOnly, changed behavior such that if there are no changes, the original reader is not set to readonly (the previous behavior was to set readonly to true even if the reader did not have changes).
        Hide
        Michael McCandless added a comment -

        Jason I think your last patch is stale? I don't see the new test case nor the changed logic about setting reader to readOnly on clone.

        Show
        Michael McCandless added a comment - Jason I think your last patch is stale? I don't see the new test case nor the changed logic about setting reader to readOnly on clone.
        Hide
        Jason Rutherglen added a comment -

        Sorry, here it is.

        Show
        Jason Rutherglen added a comment - Sorry, here it is.
        Hide
        Michael McCandless added a comment -

        I'm seeing this failure:

        [junit] Testcase: testCloneWriteToOrig(org.apache.lucene.index.TestIndexReaderClone):	FAILED
        [junit] deleting from the original should not have worked
        [junit] junit.framework.AssertionFailedError: deleting from the original should not have worked
        [junit] 	at org.apache.lucene.index.TestIndexReaderClone.testCloneWriteToOrig(TestIndexReaderClone.java:53)
        [junit] 
        [junit] 
        [junit] Test org.apache.lucene.index.TestIndexReaderClone FAILED
        

        I think that test just needs to be updated based on the new semantics? Could you also test the reverse (that r2 is also able to do a delete, as long as r1 hasn't).

        And also this one:

        [junit] Testcase: testNormsRefCounting(org.apache.lucene.index.TestIndexReaderCloneNorms):	FAILED
        [junit] did not hit expected exception
        [junit] junit.framework.AssertionFailedError: did not hit expected exception
        [junit] 	at org.apache.lucene.index.TestIndexReaderCloneNorms.testNormsRefCounting(TestIndexReaderCloneNorms.java:179)
        [junit] 
        

        Which I think is failing for the same reason (changed semantics).

        It seems like you also now allow non-readOnly reader to clone to a writable one? Is there a test case for that?

        Also can you make sure you always close MockRAMDir's that you opened (at least one test does not)? On close it fails if there are still any open files, which is a good test that we are not over-incref'ing somewhere.

        Show
        Michael McCandless added a comment - I'm seeing this failure: [junit] Testcase: testCloneWriteToOrig(org.apache.lucene.index.TestIndexReaderClone): FAILED [junit] deleting from the original should not have worked [junit] junit.framework.AssertionFailedError: deleting from the original should not have worked [junit] at org.apache.lucene.index.TestIndexReaderClone.testCloneWriteToOrig(TestIndexReaderClone.java:53) [junit] [junit] [junit] Test org.apache.lucene.index.TestIndexReaderClone FAILED I think that test just needs to be updated based on the new semantics? Could you also test the reverse (that r2 is also able to do a delete, as long as r1 hasn't). And also this one: [junit] Testcase: testNormsRefCounting(org.apache.lucene.index.TestIndexReaderCloneNorms): FAILED [junit] did not hit expected exception [junit] junit.framework.AssertionFailedError: did not hit expected exception [junit] at org.apache.lucene.index.TestIndexReaderCloneNorms.testNormsRefCounting(TestIndexReaderCloneNorms.java:179) [junit] Which I think is failing for the same reason (changed semantics). It seems like you also now allow non-readOnly reader to clone to a writable one? Is there a test case for that? Also can you make sure you always close MockRAMDir's that you opened (at least one test does not)? On close it fails if there are still any open files, which is a good test that we are not over-incref'ing somewhere.
        Hide
        Jason Rutherglen added a comment -

        I was running "ant test-tag" which doesn't run all the tests (such as
        the ones from the patch) so the failures weren't showing up. Executed ant test-core.
        And I always see:
        [junit] Testcase:
        warning(junit.framework.TestSuite$1): FAILED
        [junit] No tests found
        in org.apache.lucene.index.TestDebug
        [junit] junit.framework.AssertionFailedError: No tests found in
        org.apache.lucene.index.TestDebug

        TestIndexReaderCloneNorms.testNormsRefCounting changed where the
        expected exception test is made, trying to write to a reader that's
        been cloned, and the clone has been updated

        Changed TestIndexReaderClone.testCloneWriteToOrig to test if deleting
        from the original reader works (it should because neither one has
        made updates and acquired the write lock at that stage)

        Show
        Jason Rutherglen added a comment - I was running "ant test-tag" which doesn't run all the tests (such as the ones from the patch) so the failures weren't showing up. Executed ant test-core. And I always see: [junit] Testcase: warning(junit.framework.TestSuite$1): FAILED [junit] No tests found in org.apache.lucene.index.TestDebug [junit] junit.framework.AssertionFailedError: No tests found in org.apache.lucene.index.TestDebug TestIndexReaderCloneNorms.testNormsRefCounting changed where the expected exception test is made, trying to write to a reader that's been cloned, and the clone has been updated Changed TestIndexReaderClone.testCloneWriteToOrig to test if deleting from the original reader works (it should because neither one has made updates and acquired the write lock at that stage)
        Hide
        Michael McCandless added a comment -

        It's best to run "ant test test-tag", which tests core, contrib and back compat.

        [junit] junit.framework.AssertionFailedError: No tests found in
        org.apache.lucene.index.TestDebug

        I'm not sure what's up with that... I don't see a TestDebug.java checked in. Maybe you have a local mod?

        Show
        Michael McCandless added a comment - It's best to run "ant test test-tag", which tests core, contrib and back compat. [junit] junit.framework.AssertionFailedError: No tests found in org.apache.lucene.index.TestDebug I'm not sure what's up with that... I don't see a TestDebug.java checked in. Maybe you have a local mod?
        Hide
        Michael McCandless added a comment -

        New patch attached. All tests pass. Changes:

        • Simplified semantics: if you clone non-readOnly reader1 to
          non-readOnly reader2, I now simply clear hasChanges & writeLock on
          reader1 and transfer them to reader2, but do not set readOnly in
          reader1. This means reader1 is free to attempt to acquire the
          write lock if it wants, and it simply fails if it's stale (ie, we
          just re-use the existing code path to catch this, rather than add
          a new check), and this way we never have a case where an existing
          reader "becomes" readOnly – it can only be born readOnly.
        • Added reopen(readOnly) (what Jason referred to above). I think
          the semantics are well defined: it returns a new reader if either
          the index has changed or readOnly is different.
        • Added test for "clone readOnly to non-readOnly" case, which
          failed, and fixed various places where we were not respecting
          "openReadOnly" correctly.
        • Share common source (ReadOnlySegmentReader.noWrite) for throwing
          exception on attempting change to a readOnly reader
        • Fixed a sneaky pre-existing bug with reopen (added test case): if
          you have a non-readOnly reader on a single segment index, then add
          a segment, then reopen it and try to do a delete w/ new reader, it
          fails. This is because we were incorrectly sharing the original
          SegmentReader instance which still had its own SegmentInfos, so it
          attempts to double-acquire write lock during a single deleteDocument
          call.
        • More tests
        Show
        Michael McCandless added a comment - New patch attached. All tests pass. Changes: Simplified semantics: if you clone non-readOnly reader1 to non-readOnly reader2, I now simply clear hasChanges & writeLock on reader1 and transfer them to reader2, but do not set readOnly in reader1. This means reader1 is free to attempt to acquire the write lock if it wants, and it simply fails if it's stale (ie, we just re-use the existing code path to catch this, rather than add a new check), and this way we never have a case where an existing reader "becomes" readOnly – it can only be born readOnly. Added reopen(readOnly) (what Jason referred to above). I think the semantics are well defined: it returns a new reader if either the index has changed or readOnly is different. Added test for "clone readOnly to non-readOnly" case, which failed, and fixed various places where we were not respecting "openReadOnly" correctly. Share common source (ReadOnlySegmentReader.noWrite) for throwing exception on attempting change to a readOnly reader Fixed a sneaky pre-existing bug with reopen (added test case): if you have a non-readOnly reader on a single segment index, then add a segment, then reopen it and try to do a delete w/ new reader, it fails. This is because we were incorrectly sharing the original SegmentReader instance which still had its own SegmentInfos, so it attempts to double-acquire write lock during a single deleteDocument call. More tests
        Hide
        Shalin Shekhar Mangar added a comment -

        Fixed a sneaky pre-existing bug with reopen

        Michael, does this affect IndexReader.deleteDocument only? Is using IndexWriter.deleteDocuments fine?

        Show
        Shalin Shekhar Mangar added a comment - Fixed a sneaky pre-existing bug with reopen Michael, does this affect IndexReader.deleteDocument only? Is using IndexWriter.deleteDocuments fine?
        Hide
        Michael McCandless added a comment -

        Michael, does this affect IndexReader.deleteDocument only? Is using IndexWriter.deleteDocuments fine?

        Correct, it's only attempted changes through the reopened IndexReader that are affected (IndexWriter is fine).

        Show
        Michael McCandless added a comment - Michael, does this affect IndexReader.deleteDocument only? Is using IndexWriter.deleteDocuments fine? Correct, it's only attempted changes through the reopened IndexReader that are affected (IndexWriter is fine).
        Hide
        Michael McCandless added a comment -

        OK, new patch. I think it's ready to commit! I'll wait a day or
        two... changes:

        • Allow reopen(readOnly=true) on a reader that has pending changes
          (added test)
        • Added CHANGES.txt entry
        • Added some missing copyrights, fixed javadocs.
        • Small cleanups, asserts, refactoring
        Show
        Michael McCandless added a comment - OK, new patch. I think it's ready to commit! I'll wait a day or two... changes: Allow reopen(readOnly=true) on a reader that has pending changes (added test) Added CHANGES.txt entry Added some missing copyrights, fixed javadocs. Small cleanups, asserts, refactoring
        Hide
        Michael McCandless added a comment -

        Committed revision 739238. Thanks Jason!

        Show
        Michael McCandless added a comment - Committed revision 739238. Thanks Jason!
        Hide
        Jason Rutherglen added a comment -

        Cool, cheers Mike!

        Show
        Jason Rutherglen added a comment - Cool, cheers Mike!
        Hide
        Jason Rutherglen added a comment -

        I'm thinking of implementing a follow on patch that performs pooling
        of the byte arrays used by the norms and deleted docs. Because the
        byte array length for a segment is always the same, they can be
        pooled.

        It is useful because if IR.clone is called repeatedly and written to,
        with the old readers thrown away, the byte arrays will accumulate as
        garbage and this could affect the garbage collector. The patch will
        most likely end up in contrib as it would use ConcurrentHashMap to
        avoid synchronization.

        Show
        Jason Rutherglen added a comment - I'm thinking of implementing a follow on patch that performs pooling of the byte arrays used by the norms and deleted docs. Because the byte array length for a segment is always the same, they can be pooled. It is useful because if IR.clone is called repeatedly and written to, with the old readers thrown away, the byte arrays will accumulate as garbage and this could affect the garbage collector. The patch will most likely end up in contrib as it would use ConcurrentHashMap to avoid synchronization.
        Hide
        Michael McCandless added a comment -

        I'm thinking of implementing a follow on patch that performs pooling
        of the byte arrays used by the norms and deleted docs.

        I think this may be premature, or maybe just out of order... I'd much rather see the incremental copy on write solution (which would not be allocating the whole array) explored first, I think?

        Show
        Michael McCandless added a comment - I'm thinking of implementing a follow on patch that performs pooling of the byte arrays used by the norms and deleted docs. I think this may be premature, or maybe just out of order... I'd much rather see the incremental copy on write solution (which would not be allocating the whole array) explored first, I think?
        Hide
        Jason Rutherglen added a comment - - edited

        Now that we've got IndexReader.getSequentialSubReaders(), users may
        try to clone the sub-readers which throws the exception below. I'd
        like to support cloning individual segment readers because some cases
        require cloning on the existing segments, without reloading the new
        segments.

        The use case is realtime search where the ram index is being flushed
        to disk in the background and we don't need IR.clone to also open the
        new segments. The old segments could be acquiring deletes as the
        background flush to disk is occurring. I suppose I could write a hack
        such that a clone is performed on the multireader, then only the old
        segments are pulled out and deleted from.

        Attached is a method TestIndexReaderClone.testCloneSubreaders that
        tests individual segment reader clone.

        java.lang.NullPointerException
        	at org.apache.lucene.index.DirectoryIndexReader.clone(DirectoryIndexReader.java:171)
        	at org.apache.lucene.index.DirectoryIndexReader.clone(DirectoryIndexReader.java:162)
        	at org.apache.lucene.index.TestIndexReaderClone.testCloneSubreaders(TestIndexReaderClone.java:46)
        
        Show
        Jason Rutherglen added a comment - - edited Now that we've got IndexReader.getSequentialSubReaders(), users may try to clone the sub-readers which throws the exception below. I'd like to support cloning individual segment readers because some cases require cloning on the existing segments, without reloading the new segments. The use case is realtime search where the ram index is being flushed to disk in the background and we don't need IR.clone to also open the new segments. The old segments could be acquiring deletes as the background flush to disk is occurring. I suppose I could write a hack such that a clone is performed on the multireader, then only the old segments are pulled out and deleted from. Attached is a method TestIndexReaderClone.testCloneSubreaders that tests individual segment reader clone. java.lang.NullPointerException at org.apache.lucene.index.DirectoryIndexReader.clone(DirectoryIndexReader.java:171) at org.apache.lucene.index.DirectoryIndexReader.clone(DirectoryIndexReader.java:162) at org.apache.lucene.index.TestIndexReaderClone.testCloneSubreaders(TestIndexReaderClone.java:46)
        Hide
        Michael McCandless added a comment -

        Attached patch. If the segmentInfos is null (because the SegmentReader is not "standalone") we now just clone the segment. If you call reopen on a non-standalone SegmentReader I throw UnsupportedOperationException.

        Jason can you test this? Thanks.

        Show
        Michael McCandless added a comment - Attached patch. If the segmentInfos is null (because the SegmentReader is not "standalone") we now just clone the segment. If you call reopen on a non-standalone SegmentReader I throw UnsupportedOperationException. Jason can you test this? Thanks.
        Hide
        Jason Rutherglen added a comment -

        Mike, your patch solved the null pointer exception. An item of
        concern is when deleteDocument is called on the sub-readers, the
        write lock is not acquired, and it's not passed from the parent
        MultiSegmentReader. In the new patch I close the subreaders
        individually, then close the MultiSegmentReader. When opening the
        index again the SegmentReader can't find the .del file. Perhaps we
        javadoc the parameters of these types of cases or there's a fix.

        java.io.FileNotFoundException: _b_1.del
        	at org.apache.lucene.store.MockRAMDirectory.openInput(MockRAMDirectory.java:246)
        	at org.apache.lucene.util.BitVector.<init>(BitVector.java:209)
        	at org.apache.lucene.index.SegmentReader.loadDeletedDocs(SegmentReader.java:591)
        	at org.apache.lucene.index.SegmentReader.initialize(SegmentReader.java:557)
        	at org.apache.lucene.index.SegmentReader.get(SegmentReader.java:495)
        	at org.apache.lucene.index.SegmentReader.get(SegmentReader.java:417)
        	at org.apache.lucene.index.MultiSegmentReader.<init>(MultiSegmentReader.java:55)
        	at org.apache.lucene.index.DirectoryIndexReader$1.doBody(DirectoryIndexReader.java:112)
        	at org.apache.lucene.index.SegmentInfos$FindSegmentsFile.run(SegmentInfos.java:688)
        	at org.apache.lucene.index.DirectoryIndexReader.open(DirectoryIndexReader.java:123)
        	at org.apache.lucene.index.IndexReader.open(IndexReader.java:316)
        	at org.apache.lucene.index.IndexReader.open(IndexReader.java:227)
        	at org.apache.lucene.index.TestIndexReaderClone.testCloneSubreaders(TestIndexReaderClone.java:427)
        
        Show
        Jason Rutherglen added a comment - Mike, your patch solved the null pointer exception. An item of concern is when deleteDocument is called on the sub-readers, the write lock is not acquired, and it's not passed from the parent MultiSegmentReader. In the new patch I close the subreaders individually, then close the MultiSegmentReader. When opening the index again the SegmentReader can't find the .del file. Perhaps we javadoc the parameters of these types of cases or there's a fix. java.io.FileNotFoundException: _b_1.del at org.apache.lucene.store.MockRAMDirectory.openInput(MockRAMDirectory.java:246) at org.apache.lucene.util.BitVector.<init>(BitVector.java:209) at org.apache.lucene.index.SegmentReader.loadDeletedDocs(SegmentReader.java:591) at org.apache.lucene.index.SegmentReader.initialize(SegmentReader.java:557) at org.apache.lucene.index.SegmentReader.get(SegmentReader.java:495) at org.apache.lucene.index.SegmentReader.get(SegmentReader.java:417) at org.apache.lucene.index.MultiSegmentReader.<init>(MultiSegmentReader.java:55) at org.apache.lucene.index.DirectoryIndexReader$1.doBody(DirectoryIndexReader.java:112) at org.apache.lucene.index.SegmentInfos$FindSegmentsFile.run(SegmentInfos.java:688) at org.apache.lucene.index.DirectoryIndexReader.open(DirectoryIndexReader.java:123) at org.apache.lucene.index.IndexReader.open(IndexReader.java:316) at org.apache.lucene.index.IndexReader.open(IndexReader.java:227) at org.apache.lucene.index.TestIndexReaderClone.testCloneSubreaders(TestIndexReaderClone.java:427)
        Hide
        Michael McCandless added a comment -

        If the SegmentReader is not standalone (does not "own" a SegmentInfos,
        and is therefore a sub-reader in a MultiSegmentReader), I think you
        should not make changes (setNorm, deleteDocument) with it?

        Ie, you should always use the "parent" MultiSegmentReader to make
        changes.

        I'll update the javadocs for getSequentialSubReaders to state this.

        Show
        Michael McCandless added a comment - If the SegmentReader is not standalone (does not "own" a SegmentInfos, and is therefore a sub-reader in a MultiSegmentReader), I think you should not make changes (setNorm, deleteDocument) with it? Ie, you should always use the "parent" MultiSegmentReader to make changes. I'll update the javadocs for getSequentialSubReaders to state this.
        Hide
        Jason Rutherglen added a comment -

        If the SegmentReader is not standalone (does not "own" a SegmentInfos,
        and is therefore a sub-reader in a MultiSegmentReader), I think you
        should not make changes (setNorm, deleteDocument) with it?

        For realtime there is a use case where a Multi*Reader needs to be
        cloned without (re)opening the new segments. This is due to running
        IW.addIndexes in a backbround thread where upon completion a
        concurrent IR.clone could unintentionally open the new segments.
        Opening the new segments may be undesirable because the transaction
        may have locked segments and so does not want to delete from the new
        segments.

        One approach is an IndexReader.cloneNoNew method. Or perhaps
        IR.clone(boolean openReadOnly, boolean openNewSegments) where
        openNewSegments defaults to true.

        Show
        Jason Rutherglen added a comment - If the SegmentReader is not standalone (does not "own" a SegmentInfos, and is therefore a sub-reader in a MultiSegmentReader), I think you should not make changes (setNorm, deleteDocument) with it? For realtime there is a use case where a Multi*Reader needs to be cloned without (re)opening the new segments. This is due to running IW.addIndexes in a backbround thread where upon completion a concurrent IR.clone could unintentionally open the new segments. Opening the new segments may be undesirable because the transaction may have locked segments and so does not want to delete from the new segments. One approach is an IndexReader.cloneNoNew method. Or perhaps IR.clone(boolean openReadOnly, boolean openNewSegments) where openNewSegments defaults to true.
        Hide
        Michael McCandless added a comment -

        For realtime there is a use case where a Multi*Reader needs to be
        cloned without (re)opening the new segments.

        I thought that's exactly what clone does (does not open the segments_N)?

        This is due to running
        IW.addIndexes in a backbround thread where upon completion a
        concurrent IR.clone could unintentionally open the new segments.

        If IW is opened with autoCommit false, then even on completely of addIndexes,
        a newly [re-]opened reader won't see the change, until IW commits?

        Show
        Michael McCandless added a comment - For realtime there is a use case where a Multi*Reader needs to be cloned without (re)opening the new segments. I thought that's exactly what clone does (does not open the segments_N)? This is due to running IW.addIndexes in a backbround thread where upon completion a concurrent IR.clone could unintentionally open the new segments. If IW is opened with autoCommit false, then even on completely of addIndexes, a newly [re-] opened reader won't see the change, until IW commits?
        Hide
        Jason Rutherglen added a comment -

        I thought that's exactly what clone does (does not open the
        segments_N)?

        LUCENE-1516 IW+IR is crowding into my thoughts on realtime and clone
        where the segmentinfos is obtained from the IW which gets updated.

        If IW is opened with autoCommit false, then even on
        completely of addIndexes, a newly [re-]opened reader won't see the
        change, until IW commits?

        Perhaps we need to update the IW.addIndexes javadoc which reads:
        "This method is transactional in how Exceptions are handled: it does
        not commit a new segments_N file until all indexes are added. This
        means if an Exception occurs (for example disk full), then either no
        indexes will have been added or they all will have been."

        I wasn't sure if commit is required to see the new indexes. Maybe we
        can add something like "When autoCommit=false commit must be called
        to make the new indexes visible to a reader".

        Show
        Jason Rutherglen added a comment - I thought that's exactly what clone does (does not open the segments_N)? LUCENE-1516 IW+IR is crowding into my thoughts on realtime and clone where the segmentinfos is obtained from the IW which gets updated. If IW is opened with autoCommit false, then even on completely of addIndexes, a newly [re-] opened reader won't see the change, until IW commits? Perhaps we need to update the IW.addIndexes javadoc which reads: "This method is transactional in how Exceptions are handled: it does not commit a new segments_N file until all indexes are added. This means if an Exception occurs (for example disk full), then either no indexes will have been added or they all will have been." I wasn't sure if commit is required to see the new indexes. Maybe we can add something like "When autoCommit=false commit must be called to make the new indexes visible to a reader".
        Hide
        Michael McCandless added a comment -

        I wasn't sure if commit is required to see the new indexes. Maybe we
        can add something like "When autoCommit=false commit must be called
        to make the new indexes visible to a reader".

        Actually, this is already described in the "autoCommit=false" javadocs at the top of IW. Any & all changes done with IW are not visible to a newly opened IR until commit() is called.

        Show
        Michael McCandless added a comment - I wasn't sure if commit is required to see the new indexes. Maybe we can add something like "When autoCommit=false commit must be called to make the new indexes visible to a reader". Actually, this is already described in the "autoCommit=false" javadocs at the top of IW. Any & all changes done with IW are not visible to a newly opened IR until commit() is called.
        Hide
        Michael McCandless added a comment -

        Committed revision 744653, to allow cloning of sub-readers.

        Show
        Michael McCandless added a comment - Committed revision 744653, to allow cloning of sub-readers.
        Hide
        Michael McCandless added a comment -

        Reopening to remember to add the missing deprecations for 2.9...

        Show
        Michael McCandless added a comment - Reopening to remember to add the missing deprecations for 2.9...
        Hide
        Michael McCandless added a comment -

        Duh, wrong issue.

        Show
        Michael McCandless added a comment - Duh, wrong issue.
        Hide
        Jason Rutherglen added a comment -

        Related to LUCENE-1516 I'm seeing a bug.

        The test case in TestIndexReaderClone.testLucene1516Bug recreates the
        bug. Basically a reader is created, incRefed, a clone is made, the
        reader is decRefed, then incRefed again where the
        reader.deletedDocsRef assert > 0 fails.

        Show
        Jason Rutherglen added a comment - Related to LUCENE-1516 I'm seeing a bug. The test case in TestIndexReaderClone.testLucene1516Bug recreates the bug. Basically a reader is created, incRefed, a clone is made, the reader is decRefed, then incRefed again where the reader.deletedDocsRef assert > 0 fails.
        Hide
        Michael McCandless added a comment -

        Hmm – good catch! In fact I think we should not be incRef/decRefing the deletedDocsRef with every incRef/decRef on the SegmentReader... I'll fix.

        Show
        Michael McCandless added a comment - Hmm – good catch! In fact I think we should not be incRef/decRefing the deletedDocsRef with every incRef/decRef on the SegmentReader... I'll fix.
        Hide
        Michael McCandless added a comment -

        Attached patch. I plan to commit in a day or two.

        OK I changed deletedDocRefs to only be inc/decRef'd once for the
        reader, not every time the reader is inc/decRef'd (which was causing
        the bug you saw).

        But then because of how a reopened SegmentReader references the
        original, the deleted docs were being unecessarily copy-on-write'd (on
        closing the first reader we would fail to decref the deletedDocsRef so
        when the 2nd reader did a delete, it would copy).

        So then I added a new coreRef to SegmentReader, instead of the
        referencedSegmengReader. I like this approach better because it
        allows the original reader to be closed, without closing the core
        objects when other readers share them.

        It also simplifies SegmentReader's inc/decRef so that they no longer
        need to be overridden.

        Also, when cloning the BitVector, I changed it to not copy count over
        – if a separate thread is changing the deleted docs at the same time,
        the count could be off.

        Back-compat tests don't pass, because TestIndexReaderReopen is
        checking internal reference counts; I'll commit fixes to tests when I
        commit.

        Show
        Michael McCandless added a comment - Attached patch. I plan to commit in a day or two. OK I changed deletedDocRefs to only be inc/decRef'd once for the reader, not every time the reader is inc/decRef'd (which was causing the bug you saw). But then because of how a reopened SegmentReader references the original, the deleted docs were being unecessarily copy-on-write'd (on closing the first reader we would fail to decref the deletedDocsRef so when the 2nd reader did a delete, it would copy). So then I added a new coreRef to SegmentReader, instead of the referencedSegmengReader. I like this approach better because it allows the original reader to be closed, without closing the core objects when other readers share them. It also simplifies SegmentReader's inc/decRef so that they no longer need to be overridden. Also, when cloning the BitVector, I changed it to not copy count over – if a separate thread is changing the deleted docs at the same time, the count could be off. Back-compat tests don't pass, because TestIndexReaderReopen is checking internal reference counts; I'll commit fixes to tests when I commit.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development