[
https://issues.apache.org/jira/browse/LUCENE-3776?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13209392#comment-13209392
]
Shai Erera commented on LUCENE-3776:
------------------------------------
Patch looks good !
*SearcherManager*
with these changes, if the app passes an IndexReader that is not
DirectoryReader, it will get ClassCastException (if asserts are disabled). Is
that ok? Perhaps it'd be better if you check that in SM's ctor and throw
IllegalArgumentException? The problem is that app cannot pass DirReader in 3x,
so this will apply to trunk only. In fact, I think that for trunk it will be
better if SM declared it expects a DirectoryReader up front?
We cannot avoid the cast in refreshIfNeeded because IR is obtained from IS, but
at least the app won't hit ClassCastExceptions after it created SM?
That kinda makes SearcherManager a DirReader only impl which is unfortunate
IMO. But I'm not sure if any IR can openIfChanged() anymore, so perhaps that's
unavoidable.
*ReferenceManager*
About close() -- do you think it'll be better to keep close() final, and
introduce a new protected closeResource()/closeInternal() that NRTManager can
override? That way, RefManagers won't accidentally override close() and forget
to call super.close()?
About afterRefresh() -- I'll admit that first I didn't understand why you need
it. Previously, it was used to warm an IndexSearcher, but now we say it's the
responsibility of SearcherFactory. I can see why it's useful for NRTManager,
and it might even help me in LUCENE-3793 ! Do you think that we should declare
that it can throw IOE? I know that if I'll use it in LUCENE-3793, I'll need
that and I'd hate to throw RuntimeException. NRTManager can still override and
not declare that. I'm just thinking that since almost all methods declare
throwing IOE, it won't be odd if we declare it too on afterRefresh(), and it's
not unlikely that afterRefresh() will do something that throws exceptions.
*NRTManager*
About openIfNeeded:
# Can you cast to DirectoryReader once? I don't know if the assert is better
than a ClassCastException ... with how the code is written, ClassCastException
is better than assert because at least it will tell the user what went wrong?
# How critical it is to declare newSearcher final? If you didn't, you could
init it to null, and only change if newReader != null. Saving 4 lines of code
(improves readability IMO -- something that I know you care about :)).
> NRTManager shouldn't expose its private SearcherManager
> -------------------------------------------------------
>
> Key: LUCENE-3776
> URL: https://issues.apache.org/jira/browse/LUCENE-3776
> Project: Lucene - Java
> Issue Type: Improvement
> Reporter: Michael McCandless
> Assignee: Michael McCandless
> Priority: Blocker
> Fix For: 3.6, 4.0
>
> Attachments: LUCENE-3776.patch
>
>
> Spinoff from LUCENE-3769.
> To actually obtain an IndexSearcher from NRTManager, it's a 2-step process
> now.
> You must .getSearcherManager(), then .acquire() from the returned
> SearcherManager.
> This is very trappy... because if the app incorrectly calls maybeReopen on
> that private SearcherManager (instead of NRTManager.maybeReopen) then it can
> unexpectedly cause threads to block forever, waiting for the necessary gen to
> become visible. This will be hard to debug... I don't like creating trappy
> APIs.
> Hopefully once LUCENE-3761 is in, we can fix NRTManager to no longer expose
> its private SM, instead subclassing ReferenceManaager.
> Or alternatively, or in addition, maybe we factor out a new interface
> (SearcherProvider or something...) that only has acquire and release methods,
> and both NRTManager and ReferenceManager/SM impl that, and we keep
> NRTManager's SM private.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
