[
https://issues.apache.org/jira/browse/LUCENE-4566?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13501869#comment-13501869
]
Shai Erera commented on LUCENE-4566:
------------------------------------
It's a real bug w/ afterRefresh() - even the javadocs state that it's called
only after a new instance has been installed. Good catch !
What branch/trunk does this patch apply to?
Few comments about the patch:
* Why do you need both afterRefresh() and afterMaybeRefresh()? Is it because
NRTManager tests failures, or was there a different reason?
* Why did you add the same test to both NRTManager and SearcherManager tests?
Isn't it enough to test it once on e.g. a dummy ReferenceManager extension? The
test only verifies that the listeners are called.
* Should Listener be renamed to RefreshListener?
* Now that you have the listener, is there a reason for a protected
afterRefresh() and afterMaybeRefresh()? Aren't the listeners enough?
** On that note, I kinda like the listeners approach, so maybe we should add a
RefreshListener and CloseListener and get rid of the protected methods? The
listeners also allow us to keep the classes final, yet still have some sort of
extension point.
* Is it really necessary to use CopyOnWriteArrayList? Do we really expect an
application will install a listener after the manager has already started and
serviced calls?
** It seems like a setup method to me, and I'm fine if we document that you
should call these methods before any other method of the class is called.
> SearcherManager.afterRefresh() issues
> -------------------------------------
>
> Key: LUCENE-4566
> URL: https://issues.apache.org/jira/browse/LUCENE-4566
> Project: Lucene - Core
> Issue Type: Bug
> Reporter: selckin
> Priority: Minor
> Attachments: LUCENE-4566.patch
>
>
> 1) ReferenceManager.doMaybeRefresh seems to call afterRefresh even if it
> didn't refresh/swap, (when newReference == null)
> 2) It would be nice if users were allowed to override
> SearcherManager.afterRefresh() to get notified when a new searcher is in
> action.
> But SearcherManager and ReaderManager are final, while NRTManager is not.
> The only way to currently hook into when a new searched is created is using
> the factory, but if you wish to do some async task then, there are no
> guarantees that acquire() will return the new searcher, so you have to pass
> it around and incRef manually. While if allowed to hook into afterRefresh you
> can just rely on acquire() & existing infra you have around it to give you
> the latest one.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
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]
