[
https://issues.apache.org/jira/browse/LUCENE-4566?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13501882#comment-13501882
]
selckin commented on LUCENE-4566:
---------------------------------
against trunk r1411996
bq. * Why do you need both afterRefresh() and afterMaybeRefresh()? Is it
because NRTManager tests failures, or was there a different reason?
Yes only reason, not very clean :(, not sure if NRT actually needs to be able
to call something after every maybeRefresh or if its a test/impl restriction
(probably latter?)
bq. * 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.
True but there wasn't an existing ReferenceManager test, and it verifies they
both call super.* in the override, but as stated in one of your next points,
this can be done differently if the classes should be final.
bq. * Should Listener be renamed to RefreshListener?
Thought of that aswel, but then it felt strange to have the afterClose in there
too.
bq. * 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.
No reason
bq. * 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.
Yeah, its a bit of a trade of, i agree it's a setup method, but i didn't like
the constructor explosion with adding it as an optional constructor parameter,
and then just having a setter with no thread visibility guarantees in a class
that's made to be used multithreaded felt wrong too, so opted for the
CopyOnWrite list, there are already other listeners this way in NRTManager.
Other option is maybe to piggy back on the refresh lock in the setter maybe. Or
just document it as you say
> 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: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
