You are viewing a plain text version of this content. The canonical link for it is here.
Posted to solr-dev@lucene.apache.org by "Yonik Seeley (JIRA)" <ji...@apache.org> on 2007/10/06 02:33:50 UTC
[jira] Created: (SOLR-374) use IndexReader.reopen
use IndexReader.reopen
----------------------
Key: SOLR-374
URL: https://issues.apache.org/jira/browse/SOLR-374
Project: Solr
Issue Type: Improvement
Reporter: Yonik Seeley
Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-374) use IndexReader.reopen
Posted by "Yonik Seeley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622406#action_12622406 ]
Yonik Seeley commented on SOLR-374:
-----------------------------------
It would not have been easy in the past, but with all of the recent changes, it should be simple.
This patch has couple off issues though:
- a race condition: the reader could be closed between the time you get it and the time you try to call reopen().
- descriptor leak: you pass closeReader=false, but no one else will close this reader.
- the last reader to be opened is the one that should be re-opened, not necessarily the currently registered one
See the getNewestSearcher() method I recently added to fix both #1 and #3
Also, I think that any test that expects the reader to be different should be changed.
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (SOLR-374) use IndexReader.reopen
Posted by "Mark Miller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mark Miller updated SOLR-374:
-----------------------------
Attachment: SOLR-374.patch
Okay. Not sure how kosher taking ownership of the Reader form SolrIndexSearcher is, but it seems the thing to do then.
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-374) use IndexReader.reopen
Posted by "Yonik Seeley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622655#action_12622655 ]
Yonik Seeley commented on SOLR-374:
-----------------------------------
Looking pretty good now... but there is a reference leak. decref() should always be called for any RefCounted instances (preferably in a finally block)
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch, SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-374) use IndexReader.reopen
Posted by "Mark Miller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622420#action_12622420 ]
Mark Miller commented on SOLR-374:
----------------------------------
* a race condition: the reader could be closed between the time you get it and the time you try to call reopen().
Ah, because of no incref...
* descriptor leak: you pass closeReader=false, but no one else will close this reader.
Dumb mistake here - made a private method public just so I could pass true and then still passed false...
>>Also, I think that any test that expects the reader to be different should be changed.
Alright, easy enough, just two I think: elevation and function tests, using the reader as a key in a map or something.
If thats all for the reopen, I've got that looking good I think, just have to take care of the tests.
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (SOLR-374) use IndexReader.reopen
Posted by "Mark Miller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622696#action_12622696 ]
markrmiller@gmail.com edited comment on SOLR-374 at 8/14/08 2:04 PM:
-----------------------------------------------------------
Hmmm...probably need a searcher lock around taking reader ownership....
or not...the incref will keep it from being closed. NM.
was (Author: markrmiller@gmail.com):
Hmmm...probably need a searcher lock around taking reader ownership....
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-374) use IndexReader.reopen
Posted by "Mark Miller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622696#action_12622696 ]
Mark Miller commented on SOLR-374:
----------------------------------
Hmmm...probably need a searcher lock around taking reader ownership....
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (SOLR-374) use IndexReader.reopen
Posted by "Mark Miller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mark Miller updated SOLR-374:
-----------------------------
Attachment: SOLR-374.patch
I'm still firming up my knowledge of this class, but I think this is right. Just switched to the incref rather than Reader ownership change.
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (SOLR-374) use IndexReader.reopen
Posted by "Yonik Seeley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Yonik Seeley resolved SOLR-374.
-------------------------------
Resolution: Fixed
Committed. Thanks Mark!
This did cause a test failure on windows, but it's the test that needs fixing: SOLR-775
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Fix For: 1.4
>
> Attachments: SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (SOLR-374) use IndexReader.reopen
Posted by "Mark Miller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mark Miller updated SOLR-374:
-----------------------------
Attachment: SOLR-374.patch
* Latest patch has a new race condition: _searcher.incref() may be called after a final _searcher.decref() closes the searcher/reader.
Right...since I shouldn't even be returning _searcher, that goes away I think
* we shouldn't need to check if _searcher==null or not... there may be searchers open that have not yet been registered.
Right....gone.
* if the reader from the newest searcher is equal to it's reopen, you return the registered searcher (which may actually be different from the newest searcher)
Right....gone.
* returning a RefCounted<SolrIndexSearcher> immediately can expose it before it was supposed to be used (before warming has completed, etc)
Right....gone.
* returning a RefCounted<SolrIndexSearcher> is not always the right thing to do - it depends on the parameters to the function.
Good point :)
So I guess the key on this patch (as you pointed out) is that it is two optimizations, and the not doing anything if the Reader hasn't changed optimization really makes things more difficult - dropping it for now, I think solves pretty much each of these issues.
I was right about the Reader and the tests as well...things passed because they were still wrong - so I have adjusted the two tests to actually change the index instead of just commit.
I think this does just the reopen correctly but I am still scanning and checking. I definitely missed were the first sync on the search lock was closing in the earlier patch...soo many braces.
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch, SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-374) use IndexReader.reopen
Posted by "Yonik Seeley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622704#action_12622704 ]
Yonik Seeley commented on SOLR-374:
-----------------------------------
bq. or not...the incref will keep it from being closed. NM.
Right. I think all you need to add is a decref()
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (SOLR-374) use IndexReader.reopen
Posted by "Otis Gospodnetic (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Otis Gospodnetic updated SOLR-374:
----------------------------------
Fix Version/s: 1.4
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Fix For: 1.4
>
> Attachments: SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Closed: (SOLR-374) use IndexReader.reopen
Posted by "Grant Ingersoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Grant Ingersoll closed SOLR-374.
--------------------------------
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Fix For: 1.4
>
> Attachments: SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-374) use IndexReader.reopen
Posted by "Mark Miller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622724#action_12622724 ]
Mark Miller commented on SOLR-374:
----------------------------------
Man...nothing is ever simple :) A search lock around the ownership change would solve that right? The incref on the Reader is way cleaner though - from what I can tell solr Lucene is a bit too old though. Worth it to wait I think - much better than a sync.
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-374) use IndexReader.reopen
Posted by "Yonik Seeley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622717#action_12622717 ]
Yonik Seeley commented on SOLR-374:
-----------------------------------
I missed the last patch (I wish JIRA defaulted to "All").
It seems like that if reopen() returns us the same reader, we should just incRef it... (or is that in a slightly later version of Lucene?)
Trying to steal the reader instead seems hard to get right (seems like another thread could try to open another searcher, but our searcher doesn't have it and neither does the old one, so your exception might be triggered.)
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-374) use IndexReader.reopen
Posted by "Mark Miller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622707#action_12622707 ]
Mark Miller commented on SOLR-374:
----------------------------------
I've got the decref on the newestSearcher in a finally block there - miss it? or did I botch it?
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch, SOLR-374.patch, SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (SOLR-374) use IndexReader.reopen
Posted by "Mark Miller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mark Miller updated SOLR-374:
-----------------------------
Attachment: SOLR-374.patch
I may be missing something but this one is pretty simple right?
The biggest issue I see is that some tests rely on getting a new Reader reference whether its needed or not (the index hasn't changed) after a commit. So while I'd like to just return the searcher when the Reader hasnt changed, those tests would have to be changed. As is, a small change should actually be cheaper than no change I think.
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: [jira] Commented: (SOLR-374) use IndexReader.reopen
Posted by Mark Miller <ma...@gmail.com>.
Yeah, it may not be so simple as I assumed eh? Really appreciate the
tips, I'll spend some real time going over things rather than relying
so much on the tests.
- Mark
On Aug 13, 2008, at 11:31 PM, "Yonik Seeley (JIRA)" <ji...@apache.org>
wrote:
>
> [ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622430#action_12622430
> ]
>
> Yonik Seeley commented on SOLR-374:
> -----------------------------------
>
> You've involved yourself in one of the more complicated methods in
> Solr ;-)
>
> - Latest patch has a new race condition: _searcher.incref() may be
> called after a final _searcher.decref() closes the searcher/reader.
> - we shouldn't need to check if _searcher==null or not... there may
> be searchers open that have not yet been registered.
> - if the reader from the *newest* searcher is equal to it's reopen,
> you return the registered searcher (which may actually be different
> from the newest searcher)
> - returning a RefCounted<SolrIndexSearcher> immediately can expose
> it before it was supposed to be used (before warming has completed,
> etc)
> - returning a RefCounted<SolrIndexSearcher> is not always the right
> thing to do - it depends on the parameters to the function.
>
> There are really two different optimizations here:
> 1) call IndexReader.reopen() and share parts of the most recently
> opened IndexReader
> 2) if the IndexReader didn't change, avoid going through warming,
> autowarming, etc and just reuse the same searcher
>
>
>
>
>> use IndexReader.reopen
>> ----------------------
>>
>> Key: SOLR-374
>> URL: https://issues.apache.org/jira/browse/SOLR-374
>> Project: Solr
>> Issue Type: Improvement
>> Reporter: Yonik Seeley
>> Attachments: SOLR-374.patch, SOLR-374.patch
>>
>>
>> Take advantage of IndexReader.reopen(): LUCENE-743
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
[jira] Commented: (SOLR-374) use IndexReader.reopen
Posted by "Yonik Seeley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622430#action_12622430 ]
Yonik Seeley commented on SOLR-374:
-----------------------------------
You've involved yourself in one of the more complicated methods in Solr ;-)
- Latest patch has a new race condition: _searcher.incref() may be called after a final _searcher.decref() closes the searcher/reader.
- we shouldn't need to check if _searcher==null or not... there may be searchers open that have not yet been registered.
- if the reader from the *newest* searcher is equal to it's reopen, you return the registered searcher (which may actually be different from the newest searcher)
- returning a RefCounted<SolrIndexSearcher> immediately can expose it before it was supposed to be used (before warming has completed, etc)
- returning a RefCounted<SolrIndexSearcher> is not always the right thing to do - it depends on the parameters to the function.
There are really two different optimizations here:
1) call IndexReader.reopen() and share parts of the most recently opened IndexReader
2) if the IndexReader didn't change, avoid going through warming, autowarming, etc and just reuse the same searcher
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (SOLR-374) use IndexReader.reopen
Posted by "Mark Miller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mark Miller updated SOLR-374:
-----------------------------
Attachment: SOLR-374.patch
Hmmm...looks like I was wrong about those tests failing just because of the same Reader - looked that way, and the expected fix worked, but doing things correctly as directed by yonik, now all the tests pass no problem.
> use IndexReader.reopen
> ----------------------
>
> Key: SOLR-374
> URL: https://issues.apache.org/jira/browse/SOLR-374
> Project: Solr
> Issue Type: Improvement
> Reporter: Yonik Seeley
> Attachments: SOLR-374.patch, SOLR-374.patch
>
>
> Take advantage of IndexReader.reopen(): LUCENE-743
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.