[
https://issues.apache.org/jira/browse/LUCENE-3776?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13209693#comment-13209693
]
Michael McCandless commented on LUCENE-3776:
--------------------------------------------
Thanks Shai.
bq. with these changes, if the app passes an IndexReader that is not
DirectoryReader, it will get ClassCastException (if asserts are disabled).
Hang on -- SM now takes either IW or Directory, from which we always
pull a DirectoryReader, right (we call DR.open ourselves)? Won't we
always have a DR in SM...?
Hmm, or do you mean the SearcherFactory could make some other
reader...? Hmm maybe we should have a hard check for that
(SearcherFactory shouldn't do that...?)
bq. 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()?
Good idea... I'll add afterClose (matches afterRefresh);
bq. 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.
Good, I'll add throws IOE.
{quote}
About openIfNeeded:
Can you cast to DirectoryReader once?
{quote}
Will do.
bq. 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?
I *think* there's no way a non-DirReader can get into NRTManager (like
SM), except for SearcherFactory.
bq. 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 ).
Not critical! Good idea...
> 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: [email protected]
For additional commands, e-mail: [email protected]
