Solr
  1. Solr
  2. SOLR-423

SolrRequestHandler close notification

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.3
    • Fix Version/s: 1.3
    • Component/s: None
    • Labels:
      None

      Description

      It may be beneficial for implementations of SolrRequestHandler to be notified that the SolrCore is closing so that they can release any resources that they may have open.

      1. SOLR-423.patch
        5 kB
        Grant Ingersoll
      2. SOLR-423-CloseHook.patch
        3 kB
        Ryan McKinley
      3. SOLR-423.patch
        6 kB
        Grant Ingersoll

        Activity

        Hide
        Grant Ingersoll added a comment -

        Draft patch that implements close() callback on the SolrRequestHandler and provides empty implementation for RequestHandlerBase.

        Obviously, this is not backward compatible since it adds a new method to an interface, but I am not sure where else to put it for now. My guess is most people extend RequestHandlerBase anyway, so it may not be that big of an issue.

        Patch hooks into the SolrCore.close() call by calling RequestHandlers.closeHandlers() similar to how it inits the handlers.

        All tests should pass. Also added a test to SolrCoreTest to explicitly check that close is being called.

        Show
        Grant Ingersoll added a comment - Draft patch that implements close() callback on the SolrRequestHandler and provides empty implementation for RequestHandlerBase. Obviously, this is not backward compatible since it adds a new method to an interface, but I am not sure where else to put it for now. My guess is most people extend RequestHandlerBase anyway, so it may not be that big of an issue. Patch hooks into the SolrCore.close() call by calling RequestHandlers.closeHandlers() similar to how it inits the handlers. All tests should pass. Also added a test to SolrCoreTest to explicitly check that close is being called.
        Hide
        Ryan McKinley added a comment - - edited

        Aaaah that freaking interface! So far, we have not broken API compatibility in 1.3 (though I have tried!) so I think we ought to find another way.

        Off hand I see two options:

        1. Add close( SolrCore core ) to the SolrCoreAware interface
        2. Put the close method in RequestHandlerBase and have RequestHandlers.closeHandlers() check instanceof

        #1 seems interesting because it would also give access to close() for QueryResponseWriters and SearchComponents


        side note, where you have:

         for (Iterator<SolrRequestHandler> objectIterator = handlers.values().iterator(); objectIterator.hasNext();) {
              SolrRequestHandler handler = objectIterator.next();
              handler.close();
           }
        

        with java 1.5 this could be:

        for( SolrRequestHandler handler : handlers ) {
          handler.close();
        }
        
        Show
        Ryan McKinley added a comment - - edited Aaaah that freaking interface! So far, we have not broken API compatibility in 1.3 (though I have tried!) so I think we ought to find another way. Off hand I see two options: 1. Add close( SolrCore core ) to the SolrCoreAware interface 2. Put the close method in RequestHandlerBase and have RequestHandlers.closeHandlers() check instanceof #1 seems interesting because it would also give access to close() for QueryResponseWriters and SearchComponents side note, where you have: for (Iterator<SolrRequestHandler> objectIterator = handlers.values().iterator(); objectIterator.hasNext();) { SolrRequestHandler handler = objectIterator.next(); handler.close(); } with java 1.5 this could be: for ( SolrRequestHandler handler : handlers ) { handler.close(); }
        Hide
        Hoss Man added a comment -

        i was thinking the same thing about #1 but that got me thinking that instead of SolrCoreAware having a close method, SolrCore could have an "addCloseHook(CloseHook) method which SolrCoreAware request handlers could call as part their inform() method.

        That then got me thinking that instead of a CloseHook we should probably have a generic "closeCore" event type ... users could configure explicit SolrEventListeners in the solrconfig.xml, and plugins could use the internal methods to add implicit event listeners that they need.

        Show
        Hoss Man added a comment - i was thinking the same thing about #1 but that got me thinking that instead of SolrCoreAware having a close method, SolrCore could have an "addCloseHook(CloseHook) method which SolrCoreAware request handlers could call as part their inform() method. That then got me thinking that instead of a CloseHook we should probably have a generic "closeCore" event type ... users could configure explicit SolrEventListeners in the solrconfig.xml, and plugins could use the internal methods to add implicit event listeners that they need.
        Hide
        Ryan McKinley added a comment -

        This version adds a CloseHook to SolrCore. The test now looks like:

        class ClosingRequestHandler extends EmptyRequestHandler implements SolrCoreAware {
          boolean closed = false;
        
          public void inform(SolrCore core) {
            core.addCloseHook( new SolrCore.CloseHook() {
              public void close(SolrCore core) {
                closed = true;
              }
            });
          }
        }
        
        Show
        Ryan McKinley added a comment - This version adds a CloseHook to SolrCore. The test now looks like: class ClosingRequestHandler extends EmptyRequestHandler implements SolrCoreAware { boolean closed = false ; public void inform(SolrCore core) { core.addCloseHook( new SolrCore.CloseHook() { public void close(SolrCore core) { closed = true ; } }); } }
        Hide
        Ryan McKinley added a comment -

        > That then got me thinking that instead of a CloseHook we should probably have a generic "closeCore" event type ... users could configure explicit SolrEventListeners in the solrconfig.xml, and plugins could use the internal methods to add implicit event listeners that they need.

        How would this work? I don't quite follow. If we add close() to SolrEventListener that would break the API.

        Show
        Ryan McKinley added a comment - > That then got me thinking that instead of a CloseHook we should probably have a generic "closeCore" event type ... users could configure explicit SolrEventListeners in the solrconfig.xml, and plugins could use the internal methods to add implicit event listeners that they need. How would this work? I don't quite follow. If we add close() to SolrEventListener that would break the API.
        Hide
        Hoss Man added a comment -

        > How would this work? I don't quite follow. If we add close() to
        > SolrEventListener that would break the API.

        ...sigh.... you're 100% right. It's been a while since i looked at that API, i forgot it was an interface (heck: i forgot the event types were different methods) oh well.

        Show
        Hoss Man added a comment - > How would this work? I don't quite follow. If we add close() to > SolrEventListener that would break the API. ...sigh.... you're 100% right. It's been a while since i looked at that API, i forgot it was an interface (heck: i forgot the event types were different methods) oh well.
        Hide
        Yonik Seeley added a comment -

        SolrEventListener is a place where we might be able to make changes because no one uses their own listeners (they probably just use Solr's builtin listeners).

        Show
        Yonik Seeley added a comment - SolrEventListener is a place where we might be able to make changes because no one uses their own listeners (they probably just use Solr's builtin listeners).
        Hide
        Grant Ingersoll added a comment -

        Meant to follow up on this. I think the close hook is fine.

        Show
        Grant Ingersoll added a comment - Meant to follow up on this. I think the close hook is fine.
        Hide
        Grant Ingersoll added a comment -

        Minor updates, added some javadocs, plan to commit soon.

        Show
        Grant Ingersoll added a comment - Minor updates, added some javadocs, plan to commit soon.
        Hide
        Grant Ingersoll added a comment -

        Committed revision 671960.

        Show
        Grant Ingersoll added a comment - Committed revision 671960.

          People

          • Assignee:
            Grant Ingersoll
            Reporter:
            Grant Ingersoll
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development