Solr
  1. Solr
  2. SOLR-5783

Can we stop opening a new searcher when the index hasn't changed?

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.8, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      I've been thinking recently about how/when we re-open searchers – and what the overhead of that is in terms of caches and what not – even if the underlying index hasn't changed.

      The particular real world case that got me thinking about this recently is when a deleteByQuery gets forwarded to all shards in a collection, and then the subsequent (soft)Commit (either auto or explicit) opens a new searcher – even if that shard was completley uneffected by the delete.

      It got me wondering: why don't re-use the same searcher when the index is unchanged?

      From what I can tell, we're basically 99% of the way there (in <nrtMode/>)...

      • IndexWriter.commit is already smart enough to short circut if there's nothing to commit
      • SolrCore.openNewSearcher already uses DirectoryReader.openIfChanged to see if the reader can be re-used.
      • for "realtime" purposes, SolrCore.openNewSearcher will return the existing searcher if it exists and the DirectoryReader hasn't changed

      ...The only reason I could think of for not always re-using the same searcher when the underlying DirectoryReader is identical (ie: that last bullet above) is in the situation where the "live" schema has changed – but that seems pretty trivial to account for.

      Is there any other reason why this wouldn't be a good idea for improving performance?

      1. SOLR-5783_harden_tests.patch
        5 kB
        Hoss Man
      2. SOLR-5783.patch
        16 kB
        Hoss Man
      3. SOLR-5783.patch
        14 kB
        Hoss Man
      4. SOLR-5783.patch
        7 kB
        Mark Miller
      5. SOLR-5783.patch
        3 kB
        Hoss Man

        Issue Links

          Activity

          Hide
          Hoss Man added a comment -

          Here's a quick & dirty proof of concept path to make SolrCore.openNewSearcher return the current searcher when all of the following are true:

          • the indexConfig allows for re-open
          • there is already a current searcher open
          • the underling IndexReader is unchanged from the current searcher
          • the getLiveSchema has not changed from the current searcher.

          The patch also changes SolrCore.getSearcher to skip warming when the "newSearcher" (returned by SolrCore.openNewSearcher) and the currentSearcher are identical (there's a nocommit here to fix indenting because i wanted to keep the patch simple – all i'm doing is wrapping a bunch of existing warming code in an "if" so i didn't increase the indent yet)

          This seems to work fine, and solves the problem i've been thinking about: if you do a commit w/o any changes in the index – nothing happens. same index, same reader, same searcher.

          As is, this patch causes TestIndexSearcher.testRepopen to fail – but if i'm understanding that test correctly, this is because it's making an assumption about the index reader refcount being incremented by 1 after doing a changless commit - and in that case, with the patch, the reader refcount doesn't increase, because it's still the same searcher – so this should be pretty easy to fix.

          Obviously, besides fixing TestIndexSearcher.testRepopen, a lot of new tests should be written before commiting this to ensure that the same searcher is re-used when we expect, and not re-use when we don't – but before i go down that rabbit hole (the tests are likeley to be much more complicated then the code itself) does anyone see any problems with this idea that i'm not thinking of?

          Show
          Hoss Man added a comment - Here's a quick & dirty proof of concept path to make SolrCore.openNewSearcher return the current searcher when all of the following are true: the indexConfig allows for re-open there is already a current searcher open the underling IndexReader is unchanged from the current searcher the getLiveSchema has not changed from the current searcher. The patch also changes SolrCore.getSearcher to skip warming when the "newSearcher" (returned by SolrCore.openNewSearcher) and the currentSearcher are identical (there's a nocommit here to fix indenting because i wanted to keep the patch simple – all i'm doing is wrapping a bunch of existing warming code in an "if" so i didn't increase the indent yet) This seems to work fine, and solves the problem i've been thinking about: if you do a commit w/o any changes in the index – nothing happens. same index, same reader, same searcher. As is, this patch causes TestIndexSearcher.testRepopen to fail – but if i'm understanding that test correctly, this is because it's making an assumption about the index reader refcount being incremented by 1 after doing a changless commit - and in that case, with the patch, the reader refcount doesn't increase, because it's still the same searcher – so this should be pretty easy to fix. Obviously, besides fixing TestIndexSearcher.testRepopen, a lot of new tests should be written before commiting this to ensure that the same searcher is re-used when we expect, and not re-use when we don't – but before i go down that rabbit hole (the tests are likeley to be much more complicated then the code itself) does anyone see any problems with this idea that i'm not thinking of?
          Hide
          Mark Miller added a comment -

          LGTM. There was a spot it looks like you meant to use newName and used name and I had to fix a test to get the current tests to pass, but it all looks sound to me.

          Show
          Mark Miller added a comment - LGTM. There was a spot it looks like you meant to use newName and used name and I had to fix a test to get the current tests to pass, but it all looks sound to me.
          Hide
          Hoss Man added a comment -

          LGTM.

          thanks mark.

          looks like you meant to use newName

          nice catch.

          I had to fix a test...

          Your fix jives with that i was referring to – although i believe we can make that test more assertive: the reader refCounts should not only be the same, the searchers themselves should be identical.

          I'll work on adding some more test to increase my confidence

          Show
          Hoss Man added a comment - LGTM. thanks mark. looks like you meant to use newName nice catch. I had to fix a test... Your fix jives with that i was referring to – although i believe we can make that test more assertive: the reader refCounts should not only be the same, the searchers themselves should be identical. I'll work on adding some more test to increase my confidence
          Hide
          Hoss Man added a comment -

          Updated patch:

          • hardend the modified assertion in TestIndexSearcher.testReopen to verify it truly is the exact same searcher
          • added a new TestSearcherReuse to verify that we get the same searcher after doing various things that are No-Ops, and we get a new searcher after doing things that modify the index.

          still one nocommit: i want to make this use managed-schema and include a check that modifing the schema w/o any data cahnges results in a newSearcher (need to poke around the managed schema tests more to figure out how to do that)

          Show
          Hoss Man added a comment - Updated patch: hardend the modified assertion in TestIndexSearcher.testReopen to verify it truly is the exact same searcher added a new TestSearcherReuse to verify that we get the same searcher after doing various things that are No-Ops, and we get a new searcher after doing things that modify the index. still one nocommit: i want to make this use managed-schema and include a check that modifing the schema w/o any data cahnges results in a newSearcher (need to poke around the managed schema tests more to figure out how to do that)
          Hide
          Hoss Man added a comment -

          Updated patch with test asserting that opening a searcher after a schema change does result in a diff searcher, even if nothing in the underlying index didn't change.

          I'll commit after lunch if no one sees any problems

          Show
          Hoss Man added a comment - Updated patch with test asserting that opening a searcher after a schema change does result in a diff searcher, even if nothing in the underlying index didn't change. I'll commit after lunch if no one sees any problems
          Hide
          ASF subversion and git services added a comment -

          Commit 1573763 from hossman@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1573763 ]

          SOLR-5783: Requests to open a new searcher will now reuse the current registered searcher (w/o additional warming) if possible in situations where the underlying index has not changed

          Show
          ASF subversion and git services added a comment - Commit 1573763 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1573763 ] SOLR-5783 : Requests to open a new searcher will now reuse the current registered searcher (w/o additional warming) if possible in situations where the underlying index has not changed
          Hide
          ASF subversion and git services added a comment -

          Commit 1573777 from hossman@apache.org in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1573777 ]

          SOLR-5783: Requests to open a new searcher will now reuse the current registered searcher (w/o additional warming) if possible in situations where the underlying index has not changed (merge r1573763)

          Show
          ASF subversion and git services added a comment - Commit 1573777 from hossman@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1573777 ] SOLR-5783 : Requests to open a new searcher will now reuse the current registered searcher (w/o additional warming) if possible in situations where the underlying index has not changed (merge r1573763)
          Hide
          David Smiley added a comment -

          How do you trigger a new searcher on when the Solr instance receiving the commit isn't responsible for updating the index, such as when it's sharing an index on a shared disk with another Solr core that is doing the updates?

          Show
          David Smiley added a comment - How do you trigger a new searcher on when the Solr instance receiving the commit isn't responsible for updating the index, such as when it's sharing an index on a shared disk with another Solr core that is doing the updates?
          Hide
          Hoss Man added a comment -

          How do you trigger a new searcher on when the Solr instance receiving the commit isn't responsible for updating the index, such as when it's sharing an index on a shared disk with another Solr core that is doing the updates?

          David Smiley: That type of situation should not be affected at all – what changed here is that if DirectoryReader.openIfChanged(...) returns null to indicate that the underlying index hasn't changed, and the same reader should be re-used, then we also re-use the same searcher tha already wraps that reader. (This is why the replication tests still work with this change)

          Of course: If you see a problem with some edge case in your setup, please, PLEASE let me know ASAP so i can try to dig into it.

          Show
          Hoss Man added a comment - How do you trigger a new searcher on when the Solr instance receiving the commit isn't responsible for updating the index, such as when it's sharing an index on a shared disk with another Solr core that is doing the updates? David Smiley : That type of situation should not be affected at all – what changed here is that if DirectoryReader.openIfChanged(...) returns null to indicate that the underlying index hasn't changed, and the same reader should be re-used, then we also re-use the same searcher tha already wraps that reader. (This is why the replication tests still work with this change) Of course: If you see a problem with some edge case in your setup, please, PLEASE let me know ASAP so i can try to dig into it.
          Hide
          Yonik Seeley added a comment -

          Looking back at the original usecase:

          The particular real world case that got me thinking about this recently is when a deleteByQuery gets forwarded to all shards in a collection, and then the subsequent (soft)Commit (either auto or explicit) opens a new searcher – even if that shard was completley uneffected by the delete.

          And...

          IndexWriter.commit is already smart enough to short circut if there's nothing to commit

          If that's the case, why is there anything else to do? Oh, maybe DUH2 no longer short circuits? (I'm pretty sure I had code in there at one time to do nothing if nothing had changed)
          short circuiting at commit() rather than getSearcher/openSearcher seems a lot easier and less risky.

          Show
          Yonik Seeley added a comment - Looking back at the original usecase: The particular real world case that got me thinking about this recently is when a deleteByQuery gets forwarded to all shards in a collection, and then the subsequent (soft)Commit (either auto or explicit) opens a new searcher – even if that shard was completley uneffected by the delete. And... IndexWriter.commit is already smart enough to short circut if there's nothing to commit If that's the case, why is there anything else to do? Oh, maybe DUH2 no longer short circuits? (I'm pretty sure I had code in there at one time to do nothing if nothing had changed) short circuiting at commit() rather than getSearcher/openSearcher seems a lot easier and less risky.
          Hide
          Yonik Seeley added a comment -

          It looks like one problem is that this patch does not distinguish between realtime searchers and normal caching searchers.

          From the comments on openNewSearcher:

          • "realtime" means that we need to open quickly for a realtime view of the index, hence don't do any
          • autowarming and add to the _realtimeSearchers queue rather than the _searchers queue (so it won't
          • be used for autowarming by a future normal searcher). A "realtime" searcher will currently never
          • become "registered" (since it currently lacks caching).

          Hence a realtime searcher can be returned when a normal caching searcher was requested, and be used in the future for autowarming (which explains the stack trace Mark saw).

          Show
          Yonik Seeley added a comment - It looks like one problem is that this patch does not distinguish between realtime searchers and normal caching searchers. From the comments on openNewSearcher: "realtime" means that we need to open quickly for a realtime view of the index, hence don't do any autowarming and add to the _realtimeSearchers queue rather than the _searchers queue (so it won't be used for autowarming by a future normal searcher). A "realtime" searcher will currently never become "registered" (since it currently lacks caching). Hence a realtime searcher can be returned when a normal caching searcher was requested, and be used in the future for autowarming (which explains the stack trace Mark saw).
          Hide
          Yonik Seeley added a comment -

          Another issue that this change opens up code paths that were never executed before.
          For example: before this patch, something like register() would only be called once and there was no need for it to be idempotent (nor is it currently). Now it looks like it can be called an arbitrary number of times.

          Show
          Yonik Seeley added a comment - Another issue that this change opens up code paths that were never executed before. For example: before this patch, something like register() would only be called once and there was no need for it to be idempotent (nor is it currently). Now it looks like it can be called an arbitrary number of times.
          Hide
          Mark Miller added a comment -

          Yeah, there is another fail that may be related to this change that involves a nullpointer if i remember right. Might be related to that or another affect of the NRT searcher distinction or something else.

          Show
          Mark Miller added a comment - Yeah, there is another fail that may be related to this change that involves a nullpointer if i remember right. Might be related to that or another affect of the NRT searcher distinction or something else.
          Hide
          Hoss Man added a comment -
          • I'm not really understanding Yonik's alternative suggestion, but i don't have the code in front of me – if there is a better way to accomplish the same thing, then great.
          • I"m also not really understanding what problems Yonik & MArk are saying exist / may-exist with what got committed as part of this issue – but it should have just been a optimization, if it's causing problems we should definitely roll back.
          • I'm not really in a position to commit anything over the next few days, and then i'm going to be completely offline for over a week – so if one of you two (Yonik Seeley, Mark Miller) who understands why it's problematic could please revert this ASAP i'd really appreciate it.
          • If you guys could attach patches with tests cases (or pseudo code descriptions showing how to create test cases) demonstrating the problems you see with the current code that would be really helpful when i finally get a chance to revisit this in a few weeks.
          Show
          Hoss Man added a comment - I'm not really understanding Yonik's alternative suggestion, but i don't have the code in front of me – if there is a better way to accomplish the same thing, then great. I"m also not really understanding what problems Yonik & MArk are saying exist / may-exist with what got committed as part of this issue – but it should have just been a optimization, if it's causing problems we should definitely roll back. I'm not really in a position to commit anything over the next few days, and then i'm going to be completely offline for over a week – so if one of you two ( Yonik Seeley , Mark Miller ) who understands why it's problematic could please revert this ASAP i'd really appreciate it. If you guys could attach patches with tests cases (or pseudo code descriptions showing how to create test cases) demonstrating the problems you see with the current code that would be really helpful when i finally get a chance to revisit this in a few weeks.
          Hide
          Yonik Seeley added a comment -

          I'm not really in a position to commit anything over the next few days

          No worries...
          There are definitely two different bugs caused by this that I see, but they should both be easy to fix (and I think that's probably the easiest way forward at this point.) I'll try to get to it soon.

          Show
          Yonik Seeley added a comment - I'm not really in a position to commit anything over the next few days No worries... There are definitely two different bugs caused by this that I see, but they should both be easy to fix (and I think that's probably the easiest way forward at this point.) I'll try to get to it soon.
          Hide
          ASF subversion and git services added a comment -

          Commit 1577167 from Yonik Seeley in branch 'dev/trunk'
          [ https://svn.apache.org/r1577167 ]

          SOLR-5783: fix mixing caching + non caching searchers, fix multiple registration of same searcher

          Show
          ASF subversion and git services added a comment - Commit 1577167 from Yonik Seeley in branch 'dev/trunk' [ https://svn.apache.org/r1577167 ] SOLR-5783 : fix mixing caching + non caching searchers, fix multiple registration of same searcher
          Hide
          ASF subversion and git services added a comment -

          Commit 1577169 from Yonik Seeley in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1577169 ]

          SOLR-5783: fix mixing caching + non caching searchers, fix multiple registration of same searcher

          Show
          ASF subversion and git services added a comment - Commit 1577169 from Yonik Seeley in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1577169 ] SOLR-5783 : fix mixing caching + non caching searchers, fix multiple registration of same searcher
          Hide
          Yonik Seeley added a comment -

          I've committed what should be the correct fixes, but this issue should stay open until there's a proper test that fails w/o the last patch.

          Show
          Yonik Seeley added a comment - I've committed what should be the correct fixes, but this issue should stay open until there's a proper test that fails w/o the last patch.
          Hide
          Hoss Man added a comment -

          I've committed what should be the correct fixes, but this issue should stay open until there's a proper test that fails w/o the last patch.

          Thanks Yonik Seeley - based on your fixes, i've attached a SOLR-5783_harden_tests.patch that triggers 2 test failures if you revert r1577167 against SolrCore.java, but works with current trunk.

          I think this covers the two problems you noticed, but if i missed some aspect of your fixes that should also be tested, please let me know.

          I'll plan on committing this tomorrow baring objections

          Show
          Hoss Man added a comment - I've committed what should be the correct fixes, but this issue should stay open until there's a proper test that fails w/o the last patch. Thanks Yonik Seeley - based on your fixes, i've attached a SOLR-5783 _harden_tests.patch that triggers 2 test failures if you revert r1577167 against SolrCore.java, but works with current trunk. I think this covers the two problems you noticed, but if i missed some aspect of your fixes that should also be tested, please let me know. I'll plan on committing this tomorrow baring objections
          Hide
          ASF subversion and git services added a comment -

          Commit 1581398 from hossman@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1581398 ]

          SOLR-5783: harden tests

          Show
          ASF subversion and git services added a comment - Commit 1581398 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1581398 ] SOLR-5783 : harden tests
          Hide
          ASF subversion and git services added a comment -

          Commit 1581403 from hossman@apache.org in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1581403 ]

          SOLR-5783: harden tests (merge r1581398)

          Show
          ASF subversion and git services added a comment - Commit 1581403 from hossman@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1581403 ] SOLR-5783 : harden tests (merge r1581398)
          Hide
          Uwe Schindler added a comment -

          Close issue after release of 4.8.0

          Show
          Uwe Schindler added a comment - Close issue after release of 4.8.0

            People

            • Assignee:
              Hoss Man
              Reporter:
              Hoss Man
            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development