Lucene - Core
  1. Lucene - Core
  2. LUCENE-3488

Factor out SearcherManager from NRTManager

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.5, 4.0-ALPHA
    • Fix Version/s: 3.5, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      Currently we have NRTManager and SearcherManager while NRTManager contains a big piece of the code that is already in SearcherManager. Users are kind of forced to use NRTManager if they want to have SearcherManager goodness with NRT. The integration into NRTManager also forces you to maintain two instances even if you know you always want deletes. To me NRTManager tries to do more than necessary and mixes lots of responsibilities ie. handling searchers and handling indexing generations. NRTManager should use a SearcherManager by aggregation rather than duplicate a lot of logic. SearcherManager should have a NRT and Directory based implementation users can simply choose from.

      1. LUCENE-3488.patch
        44 kB
        Simon Willnauer
      2. LUCENE-3488.patch
        43 kB
        Michael McCandless
      3. LUCENE-3488.patch
        46 kB
        Simon Willnauer
      4. LUCENE-3488.patch
        46 kB
        Simon Willnauer

        Activity

        Hide
        Simon Willnauer added a comment -

        here is a patch that extracts a common SearcherManager abstract class providing simply static methods to get a SM instance for either Directory or IndexWriter. NRTManager handles all the versioning and provides methods to wait for generations just like it did before. However, NRTManager doesn't return IS anymore but SearcherManager instead. There are still a hand full of nocommits (javadoc) in the patch but since this is a major change to what we had before I figured I should upload it.

        Show
        Simon Willnauer added a comment - here is a patch that extracts a common SearcherManager abstract class providing simply static methods to get a SM instance for either Directory or IndexWriter. NRTManager handles all the versioning and provides methods to wait for generations just like it did before. However, NRTManager doesn't return IS anymore but SearcherManager instead. There are still a hand full of nocommits (javadoc) in the patch but since this is a major change to what we had before I figured I should upload it.
        Hide
        Jason Rutherglen added a comment -

        Great! Next step is integrating it into Solr and nuking the current atrocious Solr code.

        Show
        Jason Rutherglen added a comment - Great! Next step is integrating it into Solr and nuking the current atrocious Solr code.
        Hide
        Yonik Seeley added a comment -

        Jason, Solr's searcher management code has been around for 7 years, has been in production for 6, and has proven pretty bulletproof. It's heavily integrated into Solr's cache warming, replication, and many other features depends on it (in a way that make's it not-so-easy to just change how it's done).

        What you suggest only makes sense at a high level if you wave your hands around alot. If you get down to the details, it makes little sense.

        Show
        Yonik Seeley added a comment - Jason, Solr's searcher management code has been around for 7 years, has been in production for 6, and has proven pretty bulletproof. It's heavily integrated into Solr's cache warming, replication, and many other features depends on it (in a way that make's it not-so-easy to just change how it's done). What you suggest only makes sense at a high level if you wave your hands around alot. If you get down to the details, it makes little sense.
        Hide
        Jason Rutherglen added a comment -

        has been around for 7 years

        That's far too long. Hence the push for modules.

        Show
        Jason Rutherglen added a comment - has been around for 7 years That's far too long. Hence the push for modules.
        Hide
        Simon Willnauer added a comment -

        What you suggest only makes sense at a high level if you wave your hands around alot. If you get down to the details, it makes little sense.

        i am not saying we should but if we take this as a basis we should have never started with flex, DWPT etc. Stuff like this can only improve the quality of the product, make it lean and easier to maintain.

        Show
        Simon Willnauer added a comment - What you suggest only makes sense at a high level if you wave your hands around alot. If you get down to the details, it makes little sense. i am not saying we should but if we take this as a basis we should have never started with flex, DWPT etc. Stuff like this can only improve the quality of the product, make it lean and easier to maintain.
        Hide
        Yonik Seeley added a comment -

        if we take this as a basis we should have never started with flex, DWPT etc.

        You mean those didn't make sense to start off with?

        Anyway, I take exception to the characterization of this part of Solr's code as "atrocious", and really don't understand where it comes from or what the specific complaints are. But as always, patches welcome.

        Show
        Yonik Seeley added a comment - if we take this as a basis we should have never started with flex, DWPT etc. You mean those didn't make sense to start off with? Anyway, I take exception to the characterization of this part of Solr's code as "atrocious", and really don't understand where it comes from or what the specific complaints are. But as always, patches welcome.
        Hide
        Jason Rutherglen added a comment -

        Atrocious or perhaps horrible is:

        Lines 1041 - 1345 of [1]. Saying patches are welcome when these issues were brought up in SOLR-2193 when that gave way to SOLR-2565 which ended up being 155k plus several additional patches is ludicrous. A redesign could / should have yielded much better results. I didn't.

        1. http://svn.apache.org/viewvc/lucene/dev/trunk/solr/core/src/java/org/apache/solr/core/SolrCore.java?view=markup

        Show
        Jason Rutherglen added a comment - Atrocious or perhaps horrible is: Lines 1041 - 1345 of [1] . Saying patches are welcome when these issues were brought up in SOLR-2193 when that gave way to SOLR-2565 which ended up being 155k plus several additional patches is ludicrous. A redesign could / should have yielded much better results. I didn't. 1. http://svn.apache.org/viewvc/lucene/dev/trunk/solr/core/src/java/org/apache/solr/core/SolrCore.java?view=markup
        Hide
        Mark Miller added a comment -

        Stuff like this can only improve the quality of the product, make it lean and easier to maintain.

        Call me crazy, but I dont find these manager classes leaner or easier to maintain at all!

        Show
        Mark Miller added a comment - Stuff like this can only improve the quality of the product, make it lean and easier to maintain. Call me crazy, but I dont find these manager classes leaner or easier to maintain at all!
        Hide
        Simon Willnauer added a comment -

        Call me crazy, but I dont find these manager classes leaner or easier to maintain at all!

        see my comment: "i am not saying we should but if we take.."

        but hey as long as you can wrap your head around public RefCounted<SolrIndexSearcher> getSearcher(...) fine, I can't!

        Show
        Simon Willnauer added a comment - Call me crazy, but I dont find these manager classes leaner or easier to maintain at all! see my comment: "i am not saying we should but if we take.." but hey as long as you can wrap your head around public RefCounted<SolrIndexSearcher> getSearcher(...) fine, I can't!
        Hide
        Mark Miller added a comment -

        see my comment: "i am not saying we should but if we take.."

        I'm not arguing about that - I'm just commenting on your comment.

        but hey as long as you can wrap your head around public RefCounted<SolrIndexSearcher> getSearcher(...) fine, I can't!

        Well, if you take out all of the things that it's doing above and beyond these manager classes, sure it would be a much leaner class ... but thats more than a few features removed... removed features generally means less code.

        I'm not saying it's not a method that couldn't be broken down or refactored to be made more clear - but as someone that has written classes like these manager classes, and has worked closely on this area of Solr, I'm just not finding these comments jive with my experience. People love to jump on Solr code, but generally it's not very strong arguments. The UpdateHandler was redesigned IMO - I tackled that with an open page. If someone thinks it should be redesigned further, the proof is in the pudding. I'm happy where I took it at the moment. Wasn't a big redesign because in the end, that was not necessary - it was not a bad design to start - some of it was simply dated compared to Lucene advancements.

        Show
        Mark Miller added a comment - see my comment: "i am not saying we should but if we take.." I'm not arguing about that - I'm just commenting on your comment. but hey as long as you can wrap your head around public RefCounted<SolrIndexSearcher> getSearcher(...) fine, I can't! Well, if you take out all of the things that it's doing above and beyond these manager classes, sure it would be a much leaner class ... but thats more than a few features removed... removed features generally means less code. I'm not saying it's not a method that couldn't be broken down or refactored to be made more clear - but as someone that has written classes like these manager classes, and has worked closely on this area of Solr, I'm just not finding these comments jive with my experience. People love to jump on Solr code, but generally it's not very strong arguments. The UpdateHandler was redesigned IMO - I tackled that with an open page. If someone thinks it should be redesigned further, the proof is in the pudding. I'm happy where I took it at the moment. Wasn't a big redesign because in the end, that was not necessary - it was not a bad design to start - some of it was simply dated compared to Lucene advancements.
        Hide
        Jason Rutherglen added a comment -

        manager classes leaner

        Leaner, better, modularized, pluggable, etc ... etc.

        SolrCore is final. I remember having that debate a while back with Chris Hostetter. Why Solr needs to be monolithic, I don't know. Attempts to fix that have met with, and continue to be met with push back. That is quite evidently clear!

        Show
        Jason Rutherglen added a comment - manager classes leaner Leaner, better, modularized, pluggable, etc ... etc. SolrCore is final . I remember having that debate a while back with Chris Hostetter. Why Solr needs to be monolithic, I don't know. Attempts to fix that have met with, and continue to be met with push back. That is quite evidently clear!
        Hide
        Mark Miller added a comment -

        and continue to be met with push back.

        Arn't you used to pushback on your code / ideas by now?

        Show
        Mark Miller added a comment - and continue to be met with push back. Arn't you used to pushback on your code / ideas by now?
        Hide
        Simon Willnauer added a comment -

        Ok. lets got back to this code. non of the comments have been related to this!

        Show
        Simon Willnauer added a comment - Ok. lets got back to this code. non of the comments have been related to this!
        Hide
        Jason Rutherglen added a comment -

        Ok. lets got back to this code. non of the comments have been related to this!

        +1 to the code!

        Show
        Jason Rutherglen added a comment - Ok. lets got back to this code. non of the comments have been related to this! +1 to the code!
        Hide
        Michael McCandless added a comment -

        This is great Simon!

        I love how you now ask NRTMgr for a w/ deletes and w/o deletes
        SearcherManager, and then can just use the SearcherMgr from then on
        (optionally waiting for the right gen, on the NRTMgr, if you need to).

        Maybe we can somehow take this further... like you open a separate
        NRTMgr that doesn't care about deletes (this is super-expert anyway);
        then we don't have the "if (mustApplyDeletes)" throughout (but we can
        do this under a new issue).

        I made some small cleanups (removed some "TODO Auto-generated method
        stub"s); renamed NRTSearchManager -> NRTSearcherManager (the inner
        class in SearcherManager; added default ctor setting
        alwaysApplyDeletes=true in NRTManager; put synchronized back in
        SearcherManager.swapSearcher (ok?)).

        Show
        Michael McCandless added a comment - This is great Simon! I love how you now ask NRTMgr for a w/ deletes and w/o deletes SearcherManager, and then can just use the SearcherMgr from then on (optionally waiting for the right gen, on the NRTMgr, if you need to). Maybe we can somehow take this further... like you open a separate NRTMgr that doesn't care about deletes (this is super-expert anyway); then we don't have the "if (mustApplyDeletes)" throughout (but we can do this under a new issue). I made some small cleanups (removed some "TODO Auto-generated method stub"s); renamed NRTSearchManager -> NRTSearcherManager (the inner class in SearcherManager; added default ctor setting alwaysApplyDeletes=true in NRTManager; put synchronized back in SearcherManager.swapSearcher (ok?)).
        Hide
        Jason Rutherglen added a comment -

        Arn't you used to pushback on your code / ideas by now?

        Mark, I missed this, it's particularly funny given this issue isn't mine! Please stay on topic! (Sorry Simon, nice work!)

        Show
        Jason Rutherglen added a comment - Arn't you used to pushback on your code / ideas by now? Mark, I missed this, it's particularly funny given this issue isn't mine! Please stay on topic! (Sorry Simon, nice work!)
        Hide
        Simon Willnauer added a comment -

        Thanks mike for fixing those issues and for the cleanups! I must have missed that sync on swapSearcher. Here is the next iteration, fixing all javadocs and removes the acquireLatest and release utilities from NRTManager. Users should really use the SM instead!

        I think we are ready here!

        Show
        Simon Willnauer added a comment - Thanks mike for fixing those issues and for the cleanups! I must have missed that sync on swapSearcher. Here is the next iteration, fixing all javadocs and removes the acquireLatest and release utilities from NRTManager. Users should really use the SM instead! I think we are ready here!
        Hide
        Michael McCandless added a comment -

        I still see a few javadoc warnings... but otherwise +1 to commit; what a great simplification. It's nice that you can again pass either a Directory or Writer to SearcherManager as your "source" for new readers...

        Show
        Michael McCandless added a comment - I still see a few javadoc warnings... but otherwise +1 to commit; what a great simplification. It's nice that you can again pass either a Directory or Writer to SearcherManager as your "source" for new readers...
        Hide
        Simon Willnauer added a comment -

        final patch. fixed all javadoc warnings and added a changes entry since NRTManager was released with 3.3 and this issue changes the API quite significantly. I am going to commit this is a bit

        Show
        Simon Willnauer added a comment - final patch. fixed all javadoc warnings and added a changes entry since NRTManager was released with 3.3 and this issue changes the API quite significantly. I am going to commit this is a bit
        Hide
        Simon Willnauer added a comment -

        committed to trunk in rev: 1179956
        backported to 3.x in rev: 1179958

        Show
        Simon Willnauer added a comment - committed to trunk in rev: 1179956 backported to 3.x in rev: 1179958
        Hide
        Uwe Schindler added a comment -

        Bulk close after release of 3.5

        Show
        Uwe Schindler added a comment - Bulk close after release of 3.5

          People

          • Assignee:
            Simon Willnauer
            Reporter:
            Simon Willnauer
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development