Re: [jira] Commented: (SOLR-374) use IndexReader.reopen
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) [EMAIL PROTECTED] wrote: [ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=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 RefCountedSolrIndexSearcher immediately can expose it before it was supposed to be used (before warming has completed, etc) - returning a RefCountedSolrIndexSearcher 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
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=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
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=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] Commented: (SOLR-374) use IndexReader.reopen
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=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] Commented: (SOLR-374) use IndexReader.reopen
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=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
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=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
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=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] Commented: (SOLR-374) use IndexReader.reopen
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=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] Commented: (SOLR-374) use IndexReader.reopen
[ https://issues.apache.org/jira/browse/SOLR-374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=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 RefCountedSolrIndexSearcher immediately can expose it before it was supposed to be used (before warming has completed, etc) - returning a RefCountedSolrIndexSearcher 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.
