From b3505298a5bef76ff83b269bf87a179d027da849 Mon Sep 17 00:00:00 2001 From: Chris Hostetter Date: Tue, 2 Aug 2016 12:23:19 -0700 Subject: [PATCH] SOLR-9308: Fix distributed RTG to forward request params, fixes fq and non-default fl params --- solr/CHANGES.txt | 2 + .../component/RealTimeGetComponent.java | 47 ++++++---- .../cloud/TestCloudPseudoReturnFields.java | 91 ++++++++++--------- .../solr/cloud/TestRandomFlRTGCloud.java | 35 +++---- .../TestStressCloudBlindAtomicUpdates.java | 2 +- .../solr/search/TestPseudoReturnFields.java | 25 ++++- 6 files changed, 117 insertions(+), 85 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index cc3f6dd62b2..c2a69fdab82 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -182,6 +182,8 @@ Bug Fixes * SOLR-8379: UI Cloud->Tree view now shows .txt files correctly (Alexandre Rafalovitch via janhoy) +* SOLR-9308: Fix distributed RTG to forward request params, fixes fq and non-default fl params (hossman) + Optimizations ---------------------- diff --git a/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java b/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java index 9018a861eff..ee0a51c83ec 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java @@ -249,7 +249,8 @@ public class RealTimeGetComponent extends SearchComponent docid = segid + ctx.docBase; if (rb.getFilters() != null) { - for (Query q : rb.getFilters()) { + for (Query raw : rb.getFilters()) { + Query q = raw.rewrite(searcher.getIndexReader()); Scorer scorer = searcher.createWeight(q, false, 1f).scorer(ctx); if (scorer == null || segid != scorer.iterator().advance(segid)) { // filter doesn't match. @@ -448,7 +449,7 @@ public class RealTimeGetComponent extends SearchComponent ZkController zkController = rb.req.getCore().getCoreDescriptor().getCoreContainer().getZkController(); // if shards=... then use that - if (zkController != null && params.get("shards") == null) { + if (zkController != null && params.get(ShardParams.SHARDS) == null) { CloudDescriptor cloudDescriptor = rb.req.getCore().getCoreDescriptor().getCloudDescriptor(); String collection = cloudDescriptor.getCollectionName(); @@ -470,38 +471,46 @@ public class RealTimeGetComponent extends SearchComponent for (Map.Entry> entry : sliceToId.entrySet()) { String shard = entry.getKey(); - String shardIdList = StrUtils.join(entry.getValue(), ','); - ShardRequest sreq = new ShardRequest(); - - sreq.purpose = 1; + ShardRequest sreq = createShardRequest(rb, entry.getValue()); // sreq.shards = new String[]{shard}; // TODO: would be nice if this would work... sreq.shards = sliceToShards(rb, collection, shard); sreq.actualShards = sreq.shards; - sreq.params = new ModifiableSolrParams(); - sreq.params.set(ShardParams.SHARDS_QT,"/get"); // TODO: how to avoid hardcoding this and hit the same handler? - sreq.params.set("distrib",false); - sreq.params.set("ids", shardIdList); - + rb.addRequest(this, sreq); } } else { - String shardIdList = StrUtils.join(reqIds.allIds, ','); - ShardRequest sreq = new ShardRequest(); - - sreq.purpose = 1; + ShardRequest sreq = createShardRequest(rb, reqIds.allIds); sreq.shards = null; // ALL sreq.actualShards = sreq.shards; - sreq.params = new ModifiableSolrParams(); - sreq.params.set(ShardParams.SHARDS_QT,"/get"); // TODO: how to avoid hardcoding this and hit the same handler? - sreq.params.set("distrib",false); - sreq.params.set("ids", shardIdList); rb.addRequest(this, sreq); } return ResponseBuilder.STAGE_DONE; } + + /** + * Helper method for creating a new ShardRequest for the specified ids, based on the params + * specified for the current request. The new ShardRequest does not yet know anything about + * which shard/slice it will be sent to. + */ + private ShardRequest createShardRequest(final ResponseBuilder rb, final List ids) { + final ShardRequest sreq = new ShardRequest(); + sreq.purpose = 1; + sreq.params = new ModifiableSolrParams(rb.req.getParams()); + + // TODO: how to avoid hardcoding this and hit the same handler? + sreq.params.set(ShardParams.SHARDS_QT,"/get"); + sreq.params.set("distrib",false); + + sreq.params.remove(ShardParams.SHARDS); + sreq.params.remove("id"); + sreq.params.remove("ids"); + sreq.params.set("ids", StrUtils.join(ids, ',')); + + return sreq; + } private String[] sliceToShards(ResponseBuilder rb, String collection, String slice) { String lookup = collection + '_' + slice; // seems either form may be filled in rb.slices? diff --git a/solr/core/src/test/org/apache/solr/cloud/TestCloudPseudoReturnFields.java b/solr/core/src/test/org/apache/solr/cloud/TestCloudPseudoReturnFields.java index 8553697d6c2..6a15e682bd0 100644 --- a/solr/core/src/test/org/apache/solr/cloud/TestCloudPseudoReturnFields.java +++ b/solr/core/src/test/org/apache/solr/cloud/TestCloudPseudoReturnFields.java @@ -51,6 +51,7 @@ import org.apache.lucene.util.TestUtil; import org.apache.commons.lang.StringUtils; import org.junit.AfterClass; +import org.junit.Before; import org.junit.BeforeClass; /** @@ -104,11 +105,17 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase { assertEquals(0, CLOUD_CLIENT.add(sdoc("id", "46", "val_i", "3", "ssto", "X", "subject", "ggg")).getStatus()); assertEquals(0, CLOUD_CLIENT.commit().getStatus());; - // uncommitted doc in transaction log + } + + @Before + private void addUncommittedDoc99() throws Exception { + // uncommitted doc in transaction log at start of every test + // Even if an RTG causes ulog to re-open realtime searcher, next test method + // will get another copy of doc 99 in the ulog assertEquals(0, CLOUD_CLIENT.add(sdoc("id", "99", "val_i", "1", "ssto", "X", "subject", "uncommitted")).getStatus()); } - + @AfterClass private static void afterClass() throws Exception { CLOUD_CLIENT.close(); CLOUD_CLIENT = null; @@ -170,13 +177,12 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase { assertEquals(""+doc, 10L, doc.getFieldValue("val2_ss")); } - @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9286") public void testMultiValuedRTG() throws Exception { SolrDocument doc = null; // check same results as testMultiValued via RTG (committed doc) doc = getRandClient(random()).getById("42", params("fl","val_ss:val_i, val2_ss:10, subject")); - assertEquals(""+doc, 2, doc.size()); + assertEquals(""+doc, 3, doc.size()); assertEquals(""+doc, 1, doc.getFieldValue("val_ss")); assertEquals(""+doc, 10L, doc.getFieldValue("val2_ss")); assertEquals(""+doc, "aaa", doc.getFieldValue("subject")); @@ -218,6 +224,21 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase { } } } + + public void testFilterAndOneRealFieldRTG() throws Exception { + SolrParams params = params("fl","id,val_i", + "fq","{!field f='subject' v=$my_var}", + "my_var","uncommitted"); + SolrDocumentList docs = getRandClient(random()).getById(Arrays.asList("42","99"), params); + final String msg = params + " => " + docs; + assertEquals(msg, 1, docs.size()); + assertEquals(msg, 1, docs.getNumFound()); + + SolrDocument doc = docs.get(0); + assertEquals(msg, 2, doc.size()); + assertEquals(msg, "99", doc.getFieldValue("id")); + assertEquals(msg, 1, doc.getFieldValue("val_i")); + } public void testScoreAndAllRealFields() throws Exception { for (String fl : TestPseudoReturnFields.SCORE_AND_REAL_FIELDS) { @@ -304,7 +325,6 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase { } } - @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9286") public void testFunctionsRTG() throws Exception { // if we use RTG (committed or otherwise) functions should behave the same for (String id : Arrays.asList("42","99")) { @@ -334,7 +354,6 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase { } } - @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9286") public void testFunctionsAndExplicitRTG() throws Exception { // shouldn't matter if we use RTG (committed or otherwise) for (String id : Arrays.asList("42","99")) { @@ -382,7 +401,6 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase { } } - @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9286") public void testFunctionsAndScoreRTG() throws Exception { // if we use RTG (committed or otherwise) score should be ignored @@ -578,40 +596,35 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase { } } - @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9289") public void testDocIdAugmenterRTG() throws Exception { - // NOTE: once this test is fixed to pass, testAugmentersRTG should also be updated to test [docid] - - // TODO: in single node, [docid] is silently ignored for uncommited docs (see SOLR-9288) ... - // here we see even more confusing: [docid] is silently ignored for both committed & uncommited docs - - // behavior shouldn't matter if we are committed or uncommitted + // for an uncommitted doc, we should get -1 for (String id : Arrays.asList("42","99")) { SolrDocument doc = getRandClient(random()).getById(id, params("fl","[docid]")); String msg = id + ": fl=[docid] => " + doc; assertEquals(msg, 1, doc.size()); assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer); + assertTrue(msg, -1 <= ((Integer)doc.getFieldValue("[docid]")).intValue()); } } - @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9286") public void testAugmentersRTG() throws Exception { // behavior shouldn't matter if we are committed or uncommitted for (String id : Arrays.asList("42","99")) { - // NOTE: once testDocIdAugmenterRTG can pass, [docid] should be tested here as well. - for (SolrParams p : Arrays.asList(params("fl","[shard],[explain],x_alias:[value v=10 t=int]"), - params("fl","[shard]","fl","[explain],x_alias:[value v=10 t=int]"), - params("fl","[shard]","fl","[explain]","fl","x_alias:[value v=10 t=int]"))) { + for (SolrParams p : Arrays.asList + (params("fl","[docid],[shard],[explain],x_alias:[value v=10 t=int]"), + params("fl","[docid],[shard]","fl","[explain],x_alias:[value v=10 t=int]"), + params("fl","[docid]","fl","[shard]","fl","[explain]","fl","x_alias:[value v=10 t=int]"))) { SolrDocument doc = getRandClient(random()).getById(id, p); String msg = id + ": " + p + " => " + doc; - assertEquals(msg, 2, doc.size()); - // assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer); // TODO + assertEquals(msg, 3, doc.size()); assertTrue(msg, doc.getFieldValue("[shard]") instanceof String); // RTG: [explain] should be ignored assertTrue(msg, doc.getFieldValue("x_alias") instanceof Integer); assertEquals(msg, 10, doc.getFieldValue("x_alias")); + assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer); + assertTrue(msg, -1 <= ((Integer)doc.getFieldValue("[docid]")).intValue()); } } } @@ -635,23 +648,22 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase { } } - @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9286") public void testAugmentersAndExplicitRTG() throws Exception { // behavior shouldn't matter if we are committed or uncommitted for (String id : Arrays.asList("42","99")) { - // NOTE: once testDocIdAugmenterRTG can pass, [docid] should be tested here as well. - for (SolrParams p : Arrays.asList(params("fl","id,[explain],x_alias:[value v=10 t=int]"), - params("fl","id","fl","[explain],x_alias:[value v=10 t=int]"), - params("fl","id","fl","[explain]","fl","x_alias:[value v=10 t=int]"))) { + for (SolrParams p : Arrays.asList(params("fl","id,[docid],[explain],x_alias:[value v=10 t=int]"), + params("fl","id,[docid]","fl","[explain],x_alias:[value v=10 t=int]"), + params("fl","id","fl","[docid]","fl","[explain]","fl","x_alias:[value v=10 t=int]"))) { SolrDocument doc = getRandClient(random()).getById(id, p); String msg = id + ": " + p + " => " + doc; - assertEquals(msg, 2, doc.size()); + assertEquals(msg, 3, doc.size()); assertTrue(msg, doc.getFieldValue("id") instanceof String); - // assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer); // TODO // RTG: [explain] should be missing (ignored) assertTrue(msg, doc.getFieldValue("x_alias") instanceof Integer); assertEquals(msg, 10, doc.getFieldValue("x_alias")); + assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer); + assertTrue(msg, -1 <= ((Integer)doc.getFieldValue("[docid]")).intValue()); } } } @@ -688,32 +700,29 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase { } } - @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9286") public void testAugmentersAndScoreRTG() throws Exception { // if we use RTG (committed or otherwise) score should be ignored for (String id : Arrays.asList("42","99")) { - // NOTE: once testDocIdAugmenterRTG can pass, [docid] should be tested here as well. SolrDocument doc = getRandClient(random()).getById(id, params("fl","x_alias:[value v=10 t=int],score")); String msg = id + " => " + doc; assertEquals(msg, 1, doc.size()); - // assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer); // TODO assertTrue(msg, doc.getFieldValue("x_alias") instanceof Integer); assertEquals(msg, 10, doc.getFieldValue("x_alias")); - for (SolrParams p : Arrays.asList(params("fl","x_alias:[value v=10 t=int],[explain],score"), - params("fl","x_alias:[value v=10 t=int],[explain]","fl","score"), - params("fl","x_alias:[value v=10 t=int]","fl","[explain]","fl","score"))) { + for (SolrParams p : Arrays.asList(params("fl","d_alias:[docid],x_alias:[value v=10 t=int],[explain],score"), + params("fl","d_alias:[docid],x_alias:[value v=10 t=int],[explain]","fl","score"), + params("fl","d_alias:[docid]","fl","x_alias:[value v=10 t=int]","fl","[explain]","fl","score"))) { doc = getRandClient(random()).getById(id, p); msg = id + ": " + p + " => " + doc; - assertEquals(msg, 1, doc.size()); - assertTrue(msg, doc.getFieldValue("id") instanceof String); - // assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer); // TODO + assertEquals(msg, 2, doc.size()); assertTrue(msg, doc.getFieldValue("x_alias") instanceof Integer); assertEquals(msg, 10, doc.getFieldValue("x_alias")); // RTG: [explain] and score should be missing (ignored) + assertTrue(msg, doc.getFieldValue("d_alias") instanceof Integer); + assertTrue(msg, -1 <= ((Integer)doc.getFieldValue("d_alias")).intValue()); } } } @@ -758,8 +767,7 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase { // NOTE: 'ssto' is the missing one final List fl = Arrays.asList - // NOTE: once testDocIdAugmenterRTG can pass, [docid] should be tested here as well. - ("id","[explain]","score","val_*","subj*"); + ("id","[docid]","[explain]","score","val_*","subj*"); final int iters = atLeast(random, 10); for (int i = 0; i< iters; i++) { @@ -778,12 +786,13 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase { SolrDocument doc = getRandClient(random()).getById(id, params); String msg = id + ": " + params + " => " + doc; - assertEquals(msg, 3, doc.size()); + assertEquals(msg, 4, doc.size()); assertTrue(msg, doc.getFieldValue("id") instanceof String); - // assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer); // TODO assertTrue(msg, doc.getFieldValue("val_i") instanceof Integer); assertEquals(msg, 1, doc.getFieldValue("val_i")); assertTrue(msg, doc.getFieldValue("subject") instanceof String); + assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer); + assertTrue(msg, -1 <= ((Integer)doc.getFieldValue("[docid]")).intValue()); // RTG: [explain] and score should be missing (ignored) } } diff --git a/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java b/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java index 8fc61c7be0e..4c7392614a4 100644 --- a/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java +++ b/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java @@ -95,8 +95,15 @@ public class TestRandomFlRTGCloud extends SolrCloudTestCase { new GlobValidator("*_i"), new GlobValidator("*_s"), new GlobValidator("a*"), + new DocIdValidator(), + new DocIdValidator("my_docid_alias"), new SimpleFieldValueValidator("aaa_i"), new SimpleFieldValueValidator("ccc_s"), + new FunctionValidator("aaa_i"), // fq field + new FunctionValidator("aaa_i", "func_aaa_alias"), + new RenameFieldValueValidator("id", "my_id_alias"), + new RenameFieldValueValidator("bbb_i", "my_int_field_alias"), + new RenameFieldValueValidator("ddd_s", "my_str_field_alias"), new NotIncludedValidator("bogus_unused_field_ss"), new NotIncludedValidator("bogus_alias","bogus_alias:other_bogus_field_i"), new NotIncludedValidator("explain_alias","explain_alias:[explain]"), @@ -111,20 +118,12 @@ public class TestRandomFlRTGCloud extends SolrCloudTestCase { // - 50% runs use multi node/shard with FL_VALIDATORS only containing stuff that works in cloud final boolean singleCoreMode = random().nextBoolean(); if (singleCoreMode) { - // these don't work in distrib cloud mode due to SOLR-9286 - FL_VALIDATORS.addAll(Arrays.asList - (new FunctionValidator("aaa_i"), // fq field - new FunctionValidator("aaa_i", "func_aaa_alias"), - new RenameFieldValueValidator("id", "my_id_alias"), - new RenameFieldValueValidator("bbb_i", "my_int_field_alias"), - new RenameFieldValueValidator("ddd_s", "my_str_field_alias"))); - // SOLR-9289... - FL_VALIDATORS.add(new DocIdValidator()); - FL_VALIDATORS.add(new DocIdValidator("my_docid_alias")); + // No-Op + // At the moment, there are no known transformers that (we have FlValidators for and) only + // work in single core mode. } else { // No-Op // No known transformers that only work in distrib cloud but fail in singleCoreMode - } // TODO: SOLR-9314: programatically compare FL_VALIDATORS with all known transformers. // (ala QueryEqualityTest) can't be done until we eliminate the need for "singleCodeMode" @@ -301,15 +300,11 @@ public class TestRandomFlRTGCloud extends SolrCloudTestCase { // NOTE: not using SolrClient.getById or getByIds because we want to force choice of "id" vs "ids" params final ModifiableSolrParams params = params("qt","/get"); - // TODO: fq testing blocked by SOLR-9308 - // - // // random fq -- nothing fancy, secondary concern for our test - final Integer FQ_MAX = null; // TODO: replace this... - // final Integer FQ_MAX = usually() ? null : random().nextInt(); // ... with this - // if (null != FQ_MAX) { - // params.add("fq", "aaa_i:[* TO " + FQ_MAX + "]"); - // } - // TODO: END + // random fq -- nothing fancy, secondary concern for our test + final Integer FQ_MAX = usually() ? null : random().nextInt(); + if (null != FQ_MAX) { + params.add("fq", "aaa_i:[* TO " + FQ_MAX + "]"); + } final Set validators = new HashSet<>(); validators.add(ID_VALIDATOR); // always include id so we can be confident which doc we're looking at diff --git a/solr/core/src/test/org/apache/solr/cloud/TestStressCloudBlindAtomicUpdates.java b/solr/core/src/test/org/apache/solr/cloud/TestStressCloudBlindAtomicUpdates.java index 637f756bb1e..fc6d18c0605 100644 --- a/solr/core/src/test/org/apache/solr/cloud/TestStressCloudBlindAtomicUpdates.java +++ b/solr/core/src/test/org/apache/solr/cloud/TestStressCloudBlindAtomicUpdates.java @@ -320,7 +320,7 @@ public class TestStressCloudBlindAtomicUpdates extends SolrCloudTestCase { // sometimes include an fq on the expected value to ensure the updated values // are "visible" for searching final SolrParams p = (0 != TestUtil.nextInt(random(), 0,15)) - ? params() : params("fq",numericFieldName + ":" + expect); + ? params() : params("fq",numericFieldName + ":\"" + expect + "\""); SolrDocument doc = getRandClient(random()).getById(docId, p); final boolean foundWithFilter = (null != doc); diff --git a/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java b/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java index 87f3d89c021..0a987345ed2 100644 --- a/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java +++ b/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java @@ -30,6 +30,7 @@ import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.commons.lang.StringUtils; +import org.junit.Before; import org.junit.BeforeClass; @@ -61,9 +62,14 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { assertU(adoc("id", "46", "val_i", "3", "ssto", "X", "subject", "ggg")); assertU(commit()); - // uncommitted doc in transaction log + } + + @Before + private void addUncommittedDoc99() throws Exception { + // uncommitted doc in transaction log at start of every test + // Even if an RTG causes ulog to re-open realtime searcher, next test method + // will get another copy of doc 99 in the ulog assertU(adoc("id", "99", "val_i", "1", "ssto", "X", "subject", "uncommitted")); - } public void testMultiValued() throws Exception { @@ -140,8 +146,19 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { ); } } - - + } + + public void testFilterAndOneRealFieldRTG() throws Exception { + // shouldn't matter if we use RTG (committed or otherwise) + // only one of these docs should match... + assertQ("RTG w/ 2 ids & fq that only matches 1 uncommitted doc", + req("qt","/get","ids","42,99", "wt","xml","fl","id,val_i", + "fq","{!field f='subject' v=$my_var}","my_var","uncommitted") + ,"//result[@numFound='1']" + ,"//result/doc/str[@name='id'][.='99']" + ,"//result/doc/int[@name='val_i'][.='1']" + ,"//result/doc[count(*)=2]" + ); } public void testScoreAndAllRealFields() throws Exception {