[jira] [Commented] (LUCENE-3776) NRTManager shouldn't expose its private SearcherManager

["Shai Erera (Commented) (JIRA)" <ji...@apache.org>]

    [ 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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org