Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.1, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      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.

      1. LUCENE-4566.patch
        8 kB
        Michael McCandless
      2. LUCENE-4566.patch
        9 kB
        selckin
      3. LUCENE-4566-double-listeners.patch
        5 kB
        selckin

        Activity

        Hide
        Michael McCandless added a comment -

        1) seems like a bug: we should only call afterRefresh if a swap actually happened, I think?

        2) I agree this would be useful, ie a trigger after the swap has happened (SearcherFactory.newSearcher, by definition, is called before the swap has happened...). Maybe we make SM non-final and advertise that this is for afterRefresh...? Or, app passes in a separate "OnRefresh" callback interface...?

        Show
        Michael McCandless added a comment - 1) seems like a bug: we should only call afterRefresh if a swap actually happened, I think? 2) I agree this would be useful, ie a trigger after the swap has happened (SearcherFactory.newSearcher, by definition, is called before the swap has happened...). Maybe we make SM non-final and advertise that this is for afterRefresh...? Or, app passes in a separate "OnRefresh" callback interface...?
        Hide
        selckin added a comment -

        Tried to make simple patch but changing (1) breaks NRTManager tests for me and don't really have time to try to figure out how all that works

        Show
        selckin added a comment - Tried to make simple patch but changing (1) breaks NRTManager tests for me and don't really have time to try to figure out how all that works
        Hide
        selckin added a comment -
        • add extra afterMaybeRefresh to "replace" afterRefresh()
        • afterRefresh() only called when swapped as in javadoc
        • adds listener for afterRefresh & afterClose
        Show
        selckin added a comment - add extra afterMaybeRefresh to "replace" afterRefresh() afterRefresh() only called when swapped as in javadoc adds listener for afterRefresh & afterClose
        Hide
        Shai Erera added a comment -

        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.
        Show
        Shai Erera added a comment - 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.
        Hide
        selckin added a comment -

        against trunk r1411996

        * 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?)

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

        * Should Listener be renamed to RefreshListener?

        Thought of that aswel, but then it felt strange to have the afterClose in there too.

        * 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

        * 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

        Show
        selckin added a comment - against trunk r1411996 * 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?) * 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. * Should Listener be renamed to RefreshListener? Thought of that aswel, but then it felt strange to have the afterClose in there too. * 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 * 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
        Hide
        selckin added a comment -

        Simpler patch, extra Refresh & CloseListener, no changes on current afterRefresh()

        (no tests)

        Show
        selckin added a comment - Simpler patch, extra Refresh & CloseListener, no changes on current afterRefresh() (no tests)
        Hide
        Michael McCandless added a comment -

        Why do we need afterClose in the listener? It seems like the app can handle this itself? I think for NRTManager we should just keep using the protected method ...

        I think we don't need a protected method afterRefresh? It should just be private, and it invokes the listeners?

        Can we just use sync'd list for the listeners (eg like SegmentCoreReader's listeners)?

        Show
        Michael McCandless added a comment - Why do we need afterClose in the listener? It seems like the app can handle this itself? I think for NRTManager we should just keep using the protected method ... I think we don't need a protected method afterRefresh? It should just be private, and it invokes the listeners? Can we just use sync'd list for the listeners (eg like SegmentCoreReader's listeners)?
        Hide
        Michael McCandless added a comment -

        Patch, removing the closed listener (I think we don't need it?) ... I think it's ready.

        Show
        Michael McCandless added a comment - Patch, removing the closed listener (I think we don't need it?) ... I think it's ready.
        Hide
        Commit Tag Bot added a comment -

        [trunk commit] Michael McCandless
        http://svn.apache.org/viewvc?view=revision&revision=1415996

        LUCENE-4566: add RefreshListener to Reference/Searcher/NRTManager

        Show
        Commit Tag Bot added a comment - [trunk commit] Michael McCandless http://svn.apache.org/viewvc?view=revision&revision=1415996 LUCENE-4566 : add RefreshListener to Reference/Searcher/NRTManager
        Hide
        Michael McCandless added a comment -

        Thanks selckin!

        Show
        Michael McCandless added a comment - Thanks selckin!
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] Michael McCandless
        http://svn.apache.org/viewvc?view=revision&revision=1415999

        LUCENE-4566: add RefreshListener to Reference/Searcher/NRTManager

        Show
        Commit Tag Bot added a comment - [branch_4x commit] Michael McCandless http://svn.apache.org/viewvc?view=revision&revision=1415999 LUCENE-4566 : add RefreshListener to Reference/Searcher/NRTManager
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] Michael McCandless
        http://svn.apache.org/viewvc?view=revision&revision=1415999

        LUCENE-4566: add RefreshListener to Reference/Searcher/NRTManager

        Show
        Commit Tag Bot added a comment - [branch_4x commit] Michael McCandless http://svn.apache.org/viewvc?view=revision&revision=1415999 LUCENE-4566 : add RefreshListener to Reference/Searcher/NRTManager

          People

          • Assignee:
            Michael McCandless
            Reporter:
            selckin
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development