Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-9934

SolrTestCase.clearIndex should ensure IndexWriter.deleteAll is called

    Details

    • Type: Test
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.4, 7.0
    • Component/s: None
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      Normal deleteByQuery commands are subject to version constraint checks due to the possibility of out of order updates, but DUH2 has special support (triggered by version=-Long.MAX_VALUE for use by tests to override these version constraints and do a low level IndexWriter.deleteAll() call. A handful of tests override SolrTestCaseJ4.clearIndex() to take advantage of this (using copy/pasted impls), but given the intended purpose/usage of SolrTestCaseJ4.clearIndex(), it seems like the the base method in SolrTestCaseJ4 should itself trigger this low level deletion, so tests get this behavior automatically.

        Activity

        Hide
        hossman Hoss Man added a comment -

        Some background...

        Once upon a time, if if you gave solr a DBQ of *\:*, it would optimize that internally into throwing away the current Index/IndexWriter and opening a brand new one from scratch. Around the time of SOLR-3559 this had to be changed (git SHA 0f808c6bcdfcb11ce1398fe3c79c9b28c851aa1c) to account for the possibility that updates could arrive out of order, and all DBQs (even *\:*) needed their versions checked against doc versions in the index.

        At the time of this change, special code was added to DUH2 so that some tests could still force the old behavior – notably in cases where tests created synthetic versions and generally broke the tlog...

          // since we make up fake versions in these tests, we can get messed up by a DBQ with a real version
          // since Solr can think following updates were reordered.
        

        Recently as part of the work ishan and I have been doing in SOLR-5944, we realized another issue with the current behavior is that even the test code is well behaved as var as versions/tlog go, and even though clearIndex is being called in @Before methods, the low level field metdata in the IndexWriter (ex: what fields have docvalues) is surviving, causing inconsistent behavior between test methods (depending on the order of the test methods)


        In my opinion, the behavior of SolrTestCaseJ4.clearIndex() should be to do the lowest possible level of "clear the index" (not just "do a *:* DBQ) so that low level IndexWriter metadata doesn't survive, and people writting unit tests aren't suprised by stuff like this in future.

        The attached patch refactors all the various copy/pasted versions of clearIndex() that take advantage of this low level delete up into SolrTestCaseJ4.clearIndex() and adds javadocs explaining how it differs from doing your own *:* DBQ.

        Show
        hossman Hoss Man added a comment - Some background... Once upon a time, if if you gave solr a DBQ of *\:* , it would optimize that internally into throwing away the current Index/IndexWriter and opening a brand new one from scratch. Around the time of SOLR-3559 this had to be changed (git SHA 0f808c6bcdfcb11ce1398fe3c79c9b28c851aa1c) to account for the possibility that updates could arrive out of order, and all DBQs (even *\:* ) needed their versions checked against doc versions in the index. At the time of this change, special code was added to DUH2 so that some tests could still force the old behavior – notably in cases where tests created synthetic versions and generally broke the tlog... // since we make up fake versions in these tests, we can get messed up by a DBQ with a real version // since Solr can think following updates were reordered. Recently as part of the work ishan and I have been doing in SOLR-5944 , we realized another issue with the current behavior is that even the test code is well behaved as var as versions/tlog go, and even though clearIndex is being called in @Before methods, the low level field metdata in the IndexWriter (ex: what fields have docvalues) is surviving, causing inconsistent behavior between test methods (depending on the order of the test methods) In my opinion, the behavior of SolrTestCaseJ4.clearIndex() should be to do the lowest possible level of "clear the index" (not just "do a *:* DBQ) so that low level IndexWriter metadata doesn't survive, and people writting unit tests aren't suprised by stuff like this in future. The attached patch refactors all the various copy/pasted versions of clearIndex() that take advantage of this low level delete up into SolrTestCaseJ4.clearIndex() and adds javadocs explaining how it differs from doing your own *:* DBQ.
        Hide
        dsmiley David Smiley added a comment -

        +1 nice explanation and javadocs

        Show
        dsmiley David Smiley added a comment - +1 nice explanation and javadocs
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1d7379b680062eca766f0410e3db7ff9e9b34cb0 in lucene-solr's branch refs/heads/master from Chris Hostetter
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1d7379b ]

        SOLR-9934: SolrTestCase.clearIndex has been improved to take advantage of low level test specific logic that clears the index metadata more completely then a normal : DBQ can due to update versioning

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1d7379b680062eca766f0410e3db7ff9e9b34cb0 in lucene-solr's branch refs/heads/master from Chris Hostetter [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1d7379b ] SOLR-9934 : SolrTestCase.clearIndex has been improved to take advantage of low level test specific logic that clears the index metadata more completely then a normal : DBQ can due to update versioning
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 24038af7fab16aabb1365f05e9fe49d4fb1540e7 in lucene-solr's branch refs/heads/branch_6x from Chris Hostetter
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=24038af ]

        SOLR-9934: SolrTestCase.clearIndex has been improved to take advantage of low level test specific logic that clears the index metadata more completely then a normal : DBQ can due to update versioning

        (cherry picked from commit 1d7379b680062eca766f0410e3db7ff9e9b34cb0)

        Show
        jira-bot ASF subversion and git services added a comment - Commit 24038af7fab16aabb1365f05e9fe49d4fb1540e7 in lucene-solr's branch refs/heads/branch_6x from Chris Hostetter [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=24038af ] SOLR-9934 : SolrTestCase.clearIndex has been improved to take advantage of low level test specific logic that clears the index metadata more completely then a normal : DBQ can due to update versioning (cherry picked from commit 1d7379b680062eca766f0410e3db7ff9e9b34cb0)
        Hide
        mdrob Mike Drob added a comment -

        Hoss Man - is it worth converting all of the other invocations of assertU(delQ(":")) into calls to clearIndex()? Based on your description, it sounds like there might be a correctness bug lurking, but I'm not sure if it's actual or theoretical.

        I can create a new issue or upload a patch to this JIRA if you think it's worthwhile.

        Show
        mdrob Mike Drob added a comment - Hoss Man - is it worth converting all of the other invocations of assertU(delQ(" : ")) into calls to clearIndex() ? Based on your description, it sounds like there might be a correctness bug lurking, but I'm not sure if it's actual or theoretical. I can create a new issue or upload a patch to this JIRA if you think it's worthwhile.
        Hide
        hossman Hoss Man added a comment - - edited

        There might be some places where delQ could/should be replaced with cleraIndex based on the intent of the call, but it shouldn't really be causing any correctness issues.

        • if a test is doing a delQ to simulate an external user doing a delQ then that's a valid and correct usage.
        • if a test is doing a delQ to "reset" test state to emulate a completely pristine solr collection, that's where cleraIndex is (now) a better choice – but it's not more correct

        For 99% of all tests the diff is academic, but places where there is a diff are when tests muck with version numbers synthetically (ie: the original reason for this special syntax), have very specific assumptions about low level internal term stats (ie: the new updatable doc values tests, or perhaps some luke-esque tests) etc...

        if you think there are tests that would be improved by switching from delQ to clearIndex in their test scafolding (ie: in Before/After methods, or when reseting some state) then sure – go ahead and open a new issue for those. But tests that do "normal" user requests, and do "normal" delq(matchalldocs) as part of that are just fine and certainly don't need .changed.


        EDIT: after a few more minutes thought, added some more clarification about the correctness question above, and this followup comment ...

        Personally: I don't know that it's worth the effort to go looking for places to make this change. My main concern was simply that if/when people write new tests, that may involve dependencies/assumptions on having a pristine index in each test method, having clearIndex work the way it does now is good, and will automatically save people headaches like the ones Ishan and I had recently.

        Show
        hossman Hoss Man added a comment - - edited There might be some places where delQ could/should be replaced with cleraIndex based on the intent of the call, but it shouldn't really be causing any correctness issues. if a test is doing a delQ to simulate an external user doing a delQ then that's a valid and correct usage. if a test is doing a delQ to "reset" test state to emulate a completely pristine solr collection, that's where cleraIndex is (now) a better choice – but it's not more correct For 99% of all tests the diff is academic, but places where there is a diff are when tests muck with version numbers synthetically (ie: the original reason for this special syntax), have very specific assumptions about low level internal term stats (ie: the new updatable doc values tests, or perhaps some luke-esque tests) etc... if you think there are tests that would be improved by switching from delQ to clearIndex in their test scafolding (ie: in Before/After methods, or when reseting some state) then sure – go ahead and open a new issue for those. But tests that do "normal" user requests, and do "normal" delq(matchalldocs) as part of that are just fine and certainly don't need .changed. EDIT: after a few more minutes thought, added some more clarification about the correctness question above, and this followup comment ... Personally: I don't know that it's worth the effort to go looking for places to make this change. My main concern was simply that if/when people write new tests, that may involve dependencies/assumptions on having a pristine index in each test method, having clearIndex work the way it does now is good, and will automatically save people headaches like the ones Ishan and I had recently.
        Hide
        mdrob Mike Drob added a comment -

        Thanks for giving a few examples of when the difference matters. The only other consideration I can think of would be if the new method is potentially more performant by virtue of using a deeper (more direct?) method. Not having measured using JSON v XML handlers, I don't know which one could make the whole test suite faster.

        Another difference that I noticed is that we don't check for success on clearIndex like we did on assertU, but maybe this doesn't matter.

        Show
        mdrob Mike Drob added a comment - Thanks for giving a few examples of when the difference matters. The only other consideration I can think of would be if the new method is potentially more performant by virtue of using a deeper (more direct?) method. Not having measured using JSON v XML handlers, I don't know which one could make the whole test suite faster. Another difference that I noticed is that we don't check for success on clearIndex like we did on assertU , but maybe this doesn't matter.

          People

          • Assignee:
            hossman Hoss Man
            Reporter:
            hossman Hoss Man
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development