Details
Improvement
Description
https://github.com/apache/lucene-solr/pull/48 has the proposed change.
Issue Links
- is required by
-
SOLR-6087
SolrIndexSearcher makes no DelegatingCollector.finish() call when IndexSearcher throws an expected exception.
-
- Closed
-
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
+1 ... looks like a great refactoring to me.
attaching patch updated to trunk ... testing now.
Hmmm.... with the patch, i'm seeing lots of tests tripping an assert in SolrIndexSearcher...
[junit4] 2> 906349 T4720 oasc.SolrException.log ERROR java.lang.AssertionError [junit4] 2> at org.apache.solr.search.SolrIndexSearcher.getDocListAndSetNC(SolrIndexSearcher.java:1701) [junit4] 2> at org.apache.solr.search.SolrIndexSearcher.getDocListC(SolrIndexSearcher.java:1427) [junit4] 2> at org.apache.solr.search.SolrIndexSearcher.access$100(SolrIndexSearcher.java:124) [junit4] 2> at org.apache.solr.search.SolrIndexSearcher$3.regenerateItem(SolrIndexSearcher.java:503) [junit4] 2> at org.apache.solr.search.LRUCache.warm(LRUCache.java:189) [junit4] 2> at org.apache.solr.search.SolrIndexSearcher.warm(SolrIndexSearcher.java:2110) [junit4] 2> at org.apache.solr.core.SolrCore$4.call(SolrCore.java:1718) [junit4] 2> at java.util.concurrent.FutureTask.run(FutureTask.java:262) ... [junit4] > Throwable #1: java.lang.AssertionError [junit4] > at __randomizedtesting.SeedInfo.seed([6B71312FB526EB2B:CE319917776B3B23]:0) [junit4] > at org.apache.solr.search.SolrIndexSearcher.getDocListAndSetNC(SolrIndexSearcher.java:1701) [junit4] > at org.apache.solr.search.SolrIndexSearcher.getDocListC(SolrIndexSearcher.java:1427) [junit4] > at org.apache.solr.search.SolrIndexSearcher.search(SolrIndexSearcher.java:512) [junit4] > at org.apache.solr.handler.component.QueryComponent.process(QueryComponent.java:461) [junit4] > at org.apache.solr.handler.component.SearchHandler.handleRequestBody(SearchHandler.java:221) [junit4] > at org.apache.solr.handler.RequestHandlerBase.handleRequest(RequestHandlerBase.java:135) [junit4] > at org.apache.solr.core.SolrCore.execute(SolrCore.java:1964) [junit4] > at org.apache.solr.util.TestHarness.query(TestHarness.java:295) [junit4] > at org.apache.solr.util.TestHarness.query(TestHarness.java:278) [junit4] > at org.apache.solr.SolrTestCaseJ4.assertQ(SolrTestCaseJ4.java:693) [junit4] > at org.apache.solr.SolrTestCaseJ4.assertQ(SolrTestCaseJ4.java:686) [junit4] > at org.apache.solr.TestTrie.testFacetField(TestTrie.java:280) [junit4] > at org.apache.solr.TestTrie.testTrieFacet_PrecisionStep(TestTrie.java:257) ... [junit4] > Throwable #1: java.lang.AssertionError [junit4] > at __randomizedtesting.SeedInfo.seed([6B71312FB526EB2B:741292EA5001AB22]:0) [junit4] > at org.apache.solr.search.SolrIndexSearcher.getDocListAndSetNC(SolrIndexSearcher.java:1701) [junit4] > at org.apache.solr.search.SolrIndexSearcher.getDocListC(SolrIndexSearcher.java:1427) [junit4] > at org.apache.solr.search.SolrIndexSearcher.search(SolrIndexSearcher.java:512) [junit4] > at org.apache.solr.handler.component.QueryComponent.process(QueryComponent.java:461) [junit4] > at org.apache.solr.handler.component.SearchHandler.handleRequestBody(SearchHandler.java:221) [junit4] > at org.apache.solr.handler.RequestHandlerBase.handleRequest(RequestHandlerBase.java:135) [junit4] > at org.apache.solr.core.SolrCore.execute(SolrCore.java:1964) [junit4] > at org.apache.solr.util.TestHarness.query(TestHarness.java:295) [junit4] > at org.apache.solr.util.TestHarness.query(TestHarness.java:278) [junit4] > at org.apache.solr.SolrTestCaseJ4.assertJQ(SolrTestCaseJ4.java:785) [junit4] > at org.apache.solr.SolrTestCaseJ4.assertJQ(SolrTestCaseJ4.java:754) [junit4] > at org.apache.solr.search.TestFiltering.testRandomFiltering(TestFiltering.java:323)
...i don't have time to dig in right now, but i'll try to circle back and review more closely later.
Hi. Thanks for reviewing and testing. I'll look into the test failures also. Could you share the exact test commands for one of the failed ones? Thank you.
...Could you share the exact test commands for one of the failed ones? Thank you.
The particular seeds/variables don't seem to matter at all...
hossman@frisbee:~/lucene/dev$ svn info | grep URL && svn update && svn status && svnversion && curl https://issues.apache.org/jira/secure/attachment/12644851/SOLR-6067.patch | patch -p0 && ant clean && cd solr/core && ant test -Dtests.seed=DEADBEEF URL: https://svn.apache.org/repos/asf/lucene/dev/trunk At revision 1594767. 1594767 ... [junit4] Tests with failures (first 10 out of 52): [junit4] - org.apache.solr.handler.component.StatsComponentTest.testStats [junit4] - org.apache.solr.handler.component.StatsComponentTest.testFieldStatisticsResultsStringField [junit4] - org.apache.solr.handler.component.StatsComponentTest.testStatsFacetMultivaluedErrorHandling [junit4] - org.apache.solr.handler.component.StatsComponentTest.testFieldStatisticsResultsNumericFieldAlwaysMissing [junit4] - org.apache.solr.handler.component.StatsComponentTest.testFieldStatisticsResultsDateField [junit4] - org.apache.solr.handler.component.StatsComponentTest.testStatsExcludeFilterQuery [junit4] - org.apache.solr.handler.component.StatsComponentTest.testFieldStatisticsResultsStringFieldAlwaysMissing [junit4] - org.apache.solr.handler.component.StatsComponentTest.testFieldStatisticsResultsDateFieldAlwaysMissing [junit4] - org.apache.solr.search.TestFiltering.testRandomFiltering [junit4] - org.apache.solr.search.TestFiltering.testCaching [junit4] [junit4] [junit4] JVM J0: 1.02 .. 1139.49 = 1138.46s [junit4] JVM J1: 1.02 .. 1133.48 = 1132.46s [junit4] JVM J2: 1.03 .. 1133.22 = 1132.18s [junit4] Execution time total: 18 minutes 59 seconds [junit4] Tests summary: 403 suites, 1681 tests, 1 suite-level error, 4 errors, 47 failures, 23 ignored (16 assumptions)
Okay, i think the problem is the last buildAndRunCollectorChain call:
In the github https://github.com/apache/lucene-solr/pull/48 diff
DocSetCollector setCollector = new DocSetDelegateCollector(maxDoc>>6, maxDoc, topCollector); buildAndRunCollectorChain(qr, query, luceneFilter, setCollector, cmd, pf.postFilter);
was simplification for
DocSetCollector setCollector = new DocSetDelegateCollector(maxDoc>>6, maxDoc, topCollector); Collector collector = setCollector; buildAndRunCollectorChain(qr, query, luceneFilter, collector, cmd, pf.postFilter);
and in the patch we see
DocSetCollector setCollector = new DocSetCollector(maxDoc>>6, maxDoc); Collector collector = MultiCollector.wrap(topCollector, setCollector); buildAndRunCollectorChain(qr, query, luceneFilter, setCollector, cmd, pf.postFilter);
i.e. the collector assignment's result is completely ignored. I'll rebase and re-push to github without use of the simplification and then hopefully patch creation will be unproblematic.
Github user cpoerschke commented on the pull request:
https://github.com/apache/lucene-solr/pull/48#issuecomment-43208284
Extra commit above relates to [comments] on SOLR-6067 ticket.
Added Grouping.java to this ticket, https://github.com/apache/lucene-solr/pull/48 updated with proposed change.
GitHub user cpoerschke opened a pull request:
https://github.com/apache/lucene-solr/pull/54
SOLR-6067: add methods to reduce code duplication in SolrIndexSearcher, Grouping
For ticket https://issues.apache.org/jira/i#browse/SOLR-6067 - baselined against trunk (previous pull request was against branch_4x).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bloomberg/lucene-solr trunk-solr-6067
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/lucene-solr/pull/54.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #54
commit 2b4d92f1192c6a4126cc69b38f5026fa4bbb7304
Author: Christine Poerschke <cpoerschke@bloomberg.net>
Date: 2014-05-08T13:23:32Z
SOLR-6067: add buildAndRunCollectorChain methods to reduce code duplication in SolrIndexSearcher, Grouping
Hi Hoss Man - i have updated with https://github.com/apache/lucene-solr/pull/54, could you try applying that patch (instead of the earlier #48 one)? Thanks.
i.e. the collector assignment's result is completely ignored.
Ah ... that was probably something i screwed up when shoehorning the patch into trunk wasn't it? sorry about that.
Added Grouping.java to this ticket, ...
Actually, I'd prefer to break that out into a separate ticket...
1) it's a refactor of similar logic, but it's not directly leveraging the other changes you've made to SolrIndexSearcher
2) unlike the way the SolrIndexSearcher refactoring is very straight forward, the Grouping changes are not so obvious to me – in particular looking at the patch i see this flow change (psuedo-code)...
if (postFilter is not null) { wrap postFilter arround allCollectors allCollectors = postFilter } if (allCollectors is not null) { execute time limited search on allCollectors }
if (allCollectors is not null) { delegate to method that: wraps allCollectors in postFilter and... execute time limited search on allCollectors }
...so basically the logic will now be very different if allCollectors is null but postFilter isn't. Off the top of my head i don't know if that actually happens, or if your code is actually more correct, but that uncertainty is why i'd prefer to tackle that separately.
—
I've attached a patch containing just the SolrIndexSearcher.java portions of your latest pull request, with some javadocs added to the new method.
Assuming tests pass, i'll commit this afternoon.
Commit 1601511 from hossman@apache.org in branch 'dev/trunk'
[ https://svn.apache.org/r1601511 ]
SOLR-6067: Refactor duplicate Collector code in SolrIndexSearcher
Commit 1601515 from hossman@apache.org in branch 'dev/branches/branch_4x'
[ https://svn.apache.org/r1601515 ]
SOLR-6067: Refactor duplicate Collector code in SolrIndexSearcher (merge r1601511)
Thanks Christine!
Feel free to open a new issue for changing/fixing hte grouping code as well - i just want to make sure it gets thought through (and ideally gets some more test cases as well) ... because if that collector can never be null, a ot of other code in that same are can be simplified (and if it can be null: then you suggested change wasn't equivalent, so we should make a conscious choice that it's a bug fix / improvement, not just a refactoring)
Hoss, the merge commit to 4x as shown by the viewvc link above appears really weird (lots of prop changes, no text change), could you check?
Actually I take it back, the text changes are there, somehow missed it in the long list. It's just that some other prop changes seem to have crept in, that's probably fine..
Thanks Hoss. Sure, the Grouping.java changes can become a separate issue.
Github user cpoerschke closed the pull request at:
Github user cpoerschke commented on the pull request:
https://github.com/apache/lucene-solr/pull/48#issuecomment-42980446
https://issues.apache.org/jira/i#browse/SOLR-6067 now opened for this change.