lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael McCandless (Commented) (JIRA)" <>
Subject [jira] [Commented] (LUCENE-3776) NRTManager shouldn't expose its private SearcherManager
Date Thu, 16 Feb 2012 20:18:59 GMT


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 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.

 About openIfNeeded:
Can you cast to DirectoryReader once?

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:
>             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:!default.jspa
For more information on JIRA, see:


To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message