Solr
  1. Solr
  2. SOLR-5168

BJQParserTest reproducible failures

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.5, 5.0
    • Component/s: None
    • Labels:
      None

      Description

      two recent Jenkins builds have uncovered some test seeds that cause failures in multiple test methods in BJQParserTest. These seeds reproduce reliably (as of trunk r1514815) ...

      ant test  -Dtestcase=BJQParserTest -Dtests.seed=7A613F321CE87F5B -Dtests.multiplier=3 -Dtests.slow=true
      
      ant test  -Dtestcase=BJQParserTest -Dtests.seed=1DC8055F837E437E -Dtests.multiplier=2 -Dtests.nightly=true -Dtests.slow=true 
      
      1. BJQTest.patch
        1 kB
        Mikhail Khludnev

        Issue Links

          Activity

          Hide
          Hoss Man added a comment -

          One of those seeds (1DC8055F837E437E) causes MockRandomMergePolicy – but a cursory review of hte test (and my cursory udnderstanding of the block join queries) doesn't suggest any reason why that should cause a problem for this test – the only ever time a commit might happen in the test is at the end of an entire block.

          The other seed (7A613F321CE87F5B) just uses LogDocMergePolicy, so even if my cusory understandings above are incorrect, there really seems to be a bug when this seed is used.

          Show
          Hoss Man added a comment - One of those seeds (1DC8055F837E437E) causes MockRandomMergePolicy – but a cursory review of hte test (and my cursory udnderstanding of the block join queries) doesn't suggest any reason why that should cause a problem for this test – the only ever time a commit might happen in the test is at the end of an entire block. The other seed (7A613F321CE87F5B) just uses LogDocMergePolicy, so even if my cusory understandings above are incorrect, there really seems to be a bug when this seed is used.
          Hide
          Mikhail Khludnev added a comment -

          I wonder, how it could work (it seems I wrote it myself - my fault).

          https://github.com/apache/lucene-solr/blob/trunk/solr/core/src/test/org/apache/solr/search/join/BJQParserTest.java#L56

          test doesn't use block add, but adds docs one by one, hence a block can be broken by commit

           public static void createIndex()
          ...
             assertU(add(doc(idDoc)));
          
          Show
          Mikhail Khludnev added a comment - I wonder, how it could work (it seems I wrote it myself - my fault). https://github.com/apache/lucene-solr/blob/trunk/solr/core/src/test/org/apache/solr/search/join/BJQParserTest.java#L56 test doesn't use block add, but adds docs one by one, hence a block can be broken by commit public static void createIndex() ... assertU(add(doc(idDoc)));
          Hide
          Yonik Seeley added a comment -

          test doesn't use block add

          Yeah, I thought that was on purpose to test the query separately from any block indexing.
          Simplest fix would be to disable the random IW stuff for this test (it would always work if the buffering in IW is enough such that the docs are flushed to a single segment). Optimizing after that fact in conjunction with the log merge policy would also work.

          Show
          Yonik Seeley added a comment - test doesn't use block add Yeah, I thought that was on purpose to test the query separately from any block indexing. Simplest fix would be to disable the random IW stuff for this test (it would always work if the buffering in IW is enough such that the docs are flushed to a single segment). Optimizing after that fact in conjunction with the log merge policy would also work.
          Hide
          Mikhail Khludnev added a comment -

          first patch. it solves most of tests but testGrandChildren() still fails on broken block.

          Show
          Mikhail Khludnev added a comment - first patch. it solves most of tests but testGrandChildren() still fails on broken block.
          Hide
          Mikhail Khludnev added a comment -

          ok. checked it again. I propose either apply that one-liner patch or trigger forceMerge() in test. After that testGrandChildren() still fails and it's fairly reasonable, though gives a subject for considerations:

          right now BJQParserTest.addGrandChildren() places grand children before their parents (i.e children). It sounds like user is aware of physical layout which is necessary for bjq and enforces it. Unfortunately, AddUpdateCommand.flatten(SolrInputDocument) reverses docs in a block quite unreasonably.
          So, there might be two vision on it:

          • we can allow users to be aware about physical layout of docs, and need to remove useless reverse in AddUpdateCommand.flatten() - not a big deal;
          • we don't want users to rely, and oblige them to nest grand children into children as well as children are nested into parents. (in this case, how to prevent them from relying on reversing?)
            Please raise a separate issue for testGrandChildren(). it can be temporarily ignored for a while.

          Thanks.

          Show
          Mikhail Khludnev added a comment - ok. checked it again. I propose either apply that one-liner patch or trigger forceMerge() in test. After that testGrandChildren() still fails and it's fairly reasonable, though gives a subject for considerations: right now BJQParserTest.addGrandChildren() places grand children before their parents (i.e children). It sounds like user is aware of physical layout which is necessary for bjq and enforces it. Unfortunately, AddUpdateCommand.flatten(SolrInputDocument) reverses docs in a block quite unreasonably. So, there might be two vision on it: we can allow users to be aware about physical layout of docs, and need to remove useless reverse in AddUpdateCommand.flatten() - not a big deal; we don't want users to rely, and oblige them to nest grand children into children as well as children are nested into parents. (in this case, how to prevent them from relying on reversing?) Please raise a separate issue for testGrandChildren(). it can be temporarily ignored for a while. Thanks.
          Hide
          Hoss Man added a comment -

          Mikhail: I don't fully understand what's going on here yet, but i know Yonik is on vaca for a week or so – so i'll go ahead and commit your patch along with an @Ignore on testGrandChildren

          we can allow users to be aware about physical layout of docs

          I don't think we should expect users to be aware of the layout. if we wnat to have low level "grey box" tests that know about those details then so be it – but the non-test code shouldn't assume end users know anything about the implementation details unless we have no other choice.

          we don't want users to rely, and oblige them to nest grand children into children as well as children are nested into parents.

          This sounds like the right course of action: if you have child docs, use addChildDocument on your parent doc; if you have grandchildren, use addChildDocument on the appropriate existing child that is the parent of the grand-child you want to add.

          Based on the current API of SolrInputDocument, I can't imagine it working any other way.

          Please raise a separate issue for testGrandChildren(). it can be temporarily ignored for a while.

          I'd prefer to leave this issue (SOLR-5168) open for now to track the test failures until they are resolved (that's the point of it's existence).

          It's not clear to me if you are saying that fixing the "grandchildren" issue requires fixing AddUpdateCommand.flatten() – in which case please open a new issue to specifically address that. (if the fix for the new issue automatically fixes this issue then great). If however the remaining problem is simply bad assumptions in this test, then we can continue to use this issue to track it.

          Show
          Hoss Man added a comment - Mikhail: I don't fully understand what's going on here yet, but i know Yonik is on vaca for a week or so – so i'll go ahead and commit your patch along with an @Ignore on testGrandChildren we can allow users to be aware about physical layout of docs I don't think we should expect users to be aware of the layout. if we wnat to have low level "grey box" tests that know about those details then so be it – but the non-test code shouldn't assume end users know anything about the implementation details unless we have no other choice. we don't want users to rely, and oblige them to nest grand children into children as well as children are nested into parents. This sounds like the right course of action: if you have child docs, use addChildDocument on your parent doc; if you have grandchildren, use addChildDocument on the appropriate existing child that is the parent of the grand-child you want to add. Based on the current API of SolrInputDocument, I can't imagine it working any other way. Please raise a separate issue for testGrandChildren(). it can be temporarily ignored for a while. I'd prefer to leave this issue ( SOLR-5168 ) open for now to track the test failures until they are resolved (that's the point of it's existence). It's not clear to me if you are saying that fixing the "grandchildren" issue requires fixing AddUpdateCommand.flatten() – in which case please open a new issue to specifically address that. (if the fix for the new issue automatically fixes this issue then great). If however the remaining problem is simply bad assumptions in this test, then we can continue to use this issue to track it.
          Hide
          ASF subversion and git services added a comment -

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

          SOLR-5168: test improvements and @Ignore still broken test

          Show
          ASF subversion and git services added a comment - Commit 1515521 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1515521 ] SOLR-5168 : test improvements and @Ignore still broken test
          Hide
          ASF subversion and git services added a comment -

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

          SOLR-5168: test improvements and @Ignore still broken test (merge r1515521)

          Show
          ASF subversion and git services added a comment - Commit 1515526 from hossman@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1515526 ] SOLR-5168 : test improvements and @Ignore still broken test (merge r1515521)
          Hide
          Mikhail Khludnev added a comment -

          fixing the "grandchildren" issue requires fixing AddUpdateCommand.flatten() – in which case please open a new issue to specifically address that. (if the fix for the new issue automatically fixes this issue then great).

          welcome SOLR-5175!

          Show
          Mikhail Khludnev added a comment - fixing the "grandchildren" issue requires fixing AddUpdateCommand.flatten() – in which case please open a new issue to specifically address that. (if the fix for the new issue automatically fixes this issue then great). welcome SOLR-5175 !
          Hide
          Mikhail Khludnev added a comment -

          pls also consider SOLR-5177

          Show
          Mikhail Khludnev added a comment - pls also consider SOLR-5177
          Show
          Mikhail Khludnev added a comment - Ignore is removed at SOLR-5175 . see https://svn.apache.org/viewvc/lucene/dev/trunk/solr/core/src/test/org/apache/solr/search/join/BJQParserTest.java?r1=1520042&r2=1520041&pathrev=1520042 feel free to close this one.
          Hide
          Yonik Seeley added a comment -

          Right - I was just running the test in a loop locally first to ensure everything was actually fixed.

          Show
          Yonik Seeley added a comment - Right - I was just running the test in a loop locally first to ensure everything was actually fixed.
          Hide
          Adrien Grand added a comment -

          4.5 release -> bulk close

          Show
          Adrien Grand added a comment - 4.5 release -> bulk close

            People

            • Assignee:
              Yonik Seeley
              Reporter:
              Hoss Man
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development