You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by sa...@apache.org on 2016/07/21 13:38:07 UTC
[45/51] [abbrv] lucene-solr:apiv2: SOLR-9309: Fix SolrCloud RTG
response structure when multi ids requested but only 1 found
SOLR-9309: Fix SolrCloud RTG response structure when multi ids requested but only 1 found
Project: http://git-wip-us.apache.org/repos/asf/lucene-solr/repo
Commit: http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/9aa639d4
Tree: http://git-wip-us.apache.org/repos/asf/lucene-solr/tree/9aa639d4
Diff: http://git-wip-us.apache.org/repos/asf/lucene-solr/diff/9aa639d4
Branch: refs/heads/apiv2
Commit: 9aa639d45e31059bb2910dade6d7728ea075cd57
Parents: 08019f4
Author: Chris Hostetter <ho...@apache.org>
Authored: Tue Jul 19 11:11:49 2016 -0700
Committer: Chris Hostetter <ho...@apache.org>
Committed: Tue Jul 19 11:11:49 2016 -0700
----------------------------------------------------------------------
solr/CHANGES.txt | 2 +
.../handler/component/RealTimeGetComponent.java | 144 ++++++++++++-------
.../apache/solr/cloud/TestRandomFlRTGCloud.java | 7 +-
3 files changed, 93 insertions(+), 60 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/9aa639d4/solr/CHANGES.txt
----------------------------------------------------------------------
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 0ccccee..55fae47 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -159,6 +159,8 @@ Bug Fixes
* SOLR-9288: Fix [docid] transformer to return -1 when used in RTG with uncommitted doc (hossman)
+* SOLR-9309: Fix SolrCloud RTG response structure when multi ids requested but only 1 found (hossman)
+
Optimizations
----------------------
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/9aa639d4/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java
----------------------------------------------------------------------
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 9865a11..9018a86 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
@@ -143,10 +143,9 @@ public class RealTimeGetComponent extends SearchComponent
return;
}
- String id[] = params.getParams("id");
- String ids[] = params.getParams("ids");
-
- if (id == null && ids == null) {
+ final IdsRequsted reqIds = IdsRequsted.parseParams(req);
+
+ if (reqIds.allIds.isEmpty()) {
return;
}
@@ -171,20 +170,6 @@ public class RealTimeGetComponent extends SearchComponent
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, e);
}
-
- String[] allIds = id==null ? new String[0] : id;
-
- if (ids != null) {
- List<String> lst = new ArrayList<>();
- for (String s : allIds) {
- lst.add(s);
- }
- for (String idList : ids) {
- lst.addAll( StrUtils.splitSmart(idList, ",", true) );
- }
- allIds = lst.toArray(new String[lst.size()]);
- }
-
SolrCore core = req.getCore();
SchemaField idField = core.getLatestSchema().getUniqueKeyField();
FieldType fieldType = idField.getType();
@@ -209,7 +194,7 @@ public class RealTimeGetComponent extends SearchComponent
SolrIndexSearcher searcher = null;
BytesRefBuilder idBytes = new BytesRefBuilder();
- for (String idStr : allIds) {
+ for (String idStr : reqIds.allIds) {
fieldType.readableToIndexed(idStr, idBytes);
if (ulog != null) {
Object o = ulog.lookup(idBytes.get());
@@ -297,18 +282,7 @@ public class RealTimeGetComponent extends SearchComponent
}
}
-
- // if the client specified a single id=foo, then use "doc":{
- // otherwise use a standard doclist
-
- if (ids == null && allIds.length <= 1) {
- // if the doc was not found, then use a value of null.
- rsp.add("doc", docList.size() > 0 ? docList.get(0) : null);
- } else {
- docList.setNumFound(docList.size());
- rsp.addResponse(docList);
- }
-
+ addDocListToResponse(rb, docList);
}
@@ -461,25 +435,13 @@ public class RealTimeGetComponent extends SearchComponent
}
public int createSubRequests(ResponseBuilder rb) throws IOException {
- SolrParams params = rb.req.getParams();
- String id1[] = params.getParams("id");
- String ids[] = params.getParams("ids");
-
- if (id1 == null && ids == null) {
+
+ final IdsRequsted reqIds = IdsRequsted.parseParams(rb.req);
+ if (reqIds.allIds.isEmpty()) {
return ResponseBuilder.STAGE_DONE;
}
-
- List<String> allIds = new ArrayList<>();
- if (id1 != null) {
- for (String s : id1) {
- allIds.add(s);
- }
- }
- if (ids != null) {
- for (String s : ids) {
- allIds.addAll( StrUtils.splitSmart(s, ",", true) );
- }
- }
+
+ SolrParams params = rb.req.getParams();
// TODO: handle collection=...?
@@ -495,7 +457,7 @@ public class RealTimeGetComponent extends SearchComponent
Map<String, List<String>> sliceToId = new HashMap<>();
- for (String id : allIds) {
+ for (String id : reqIds.allIds) {
Slice slice = coll.getRouter().getTargetSlice(id, null, null, params, coll);
List<String> idsForShard = sliceToId.get(slice.getName());
@@ -524,7 +486,7 @@ public class RealTimeGetComponent extends SearchComponent
rb.addRequest(this, sreq);
}
} else {
- String shardIdList = StrUtils.join(allIds, ',');
+ String shardIdList = StrUtils.join(reqIds.allIds, ',');
ShardRequest sreq = new ShardRequest();
sreq.purpose = 1;
@@ -586,17 +548,31 @@ public class RealTimeGetComponent extends SearchComponent
docList.addAll(subList);
}
}
+
+ addDocListToResponse(rb, docList);
+ }
- if (docList.size() <= 1 && rb.req.getParams().getParams("ids")==null) {
+ /**
+ * Encapsulates logic for how a {@link SolrDocumentList} should be added to the response
+ * based on the request params used
+ */
+ private void addDocListToResponse(final ResponseBuilder rb, final SolrDocumentList docList) {
+ assert null != docList;
+
+ final SolrQueryResponse rsp = rb.rsp;
+ final IdsRequsted reqIds = IdsRequsted.parseParams(rb.req);
+
+ if (reqIds.useSingleDocResponse) {
+ assert docList.size() <= 1;
// if the doc was not found, then use a value of null.
- rb.rsp.add("doc", docList.size() > 0 ? docList.get(0) : null);
+ rsp.add("doc", docList.size() > 0 ? docList.get(0) : null);
} else {
docList.setNumFound(docList.size());
- rb.rsp.addResponse(docList);
+ rsp.addResponse(docList);
}
}
-
+
////////////////////////////////////////////
/// SolrInfoMBean
@@ -768,6 +744,66 @@ public class RealTimeGetComponent extends SearchComponent
return new ArrayList<>(versionsToRet);
}
+ /**
+ * Simple struct for tracking what ids were requested and what response format is expected
+ * acording to the request params
+ */
+ private final static class IdsRequsted {
+ /** An List (which may be empty but will never be null) of the uniqueKeys requested. */
+ public final List<String> allIds;
+ /**
+ * true if the params provided by the user indicate that a single doc response structure
+ * should be used.
+ * Value is meaninless if <code>ids</code> is empty.
+ */
+ public final boolean useSingleDocResponse;
+ private IdsRequsted(List<String> allIds, boolean useSingleDocResponse) {
+ assert null != allIds;
+ this.allIds = allIds;
+ this.useSingleDocResponse = useSingleDocResponse;
+ }
+
+ /**
+ * Parsers the <code>id</code> and <code>ids</code> params attached to the specified request object,
+ * and returns an <code>IdsRequsted</code> struct to use for this request.
+ * The <code>IdsRequsted</code> is cached in the {@link SolrQueryRequest#getContext} so subsequent
+ * method calls on the same request will not re-parse the params.
+ */
+ public static IdsRequsted parseParams(SolrQueryRequest req) {
+ final String contextKey = IdsRequsted.class.toString() + "_PARSED_ID_PARAMS";
+ if (req.getContext().containsKey(contextKey)) {
+ return (IdsRequsted)req.getContext().get(contextKey);
+ }
+ final SolrParams params = req.getParams();
+ final String id[] = params.getParams("id");
+ final String ids[] = params.getParams("ids");
+
+ if (id == null && ids == null) {
+ IdsRequsted result = new IdsRequsted(Collections.<String>emptyList(), true);
+ req.getContext().put(contextKey, result);
+ return result;
+ }
+ final List<String> allIds = new ArrayList<>((null == id ? 0 : id.length)
+ + (null == ids ? 0 : (2 * ids.length)));
+ if (null != id) {
+ for (String singleId : id) {
+ allIds.add(singleId);
+ }
+ }
+ if (null != ids) {
+ for (String idList : ids) {
+ allIds.addAll( StrUtils.splitSmart(idList, ",", true) );
+ }
+ }
+ // if the client specified a single id=foo, then use "doc":{
+ // otherwise use a standard doclist
+ IdsRequsted result = new IdsRequsted(allIds, (ids == null && allIds.size() <= 1));
+ req.getContext().put(contextKey, result);
+ return result;
+ }
+ }
+
+
/**
* A lite weight ResultContext for use with RTG requests that can point at Realtime Searchers
*/
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/9aa639d4/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java
----------------------------------------------------------------------
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 682d6a0..8fc61c7 100644
--- a/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java
+++ b/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java
@@ -387,16 +387,11 @@ public class TestRandomFlRTGCloud extends SolrCloudTestCase {
* trivial helper method to deal with diff response structure between using a single 'id' param vs
* 2 or more 'id' params (or 1 or more 'ids' params).
*
- * NOTE: <code>expectList</code> is currently ignored due to SOLR-9309 -- instead best efforst are made to
- * return a synthetic list based on whatever can be found in the response.
- *
* @return List from response, or a synthetic one created from single response doc if
* <code>expectList</code> was false; May be empty; May be null if response included null list.
*/
private static SolrDocumentList getDocsFromRTGResponse(final boolean expectList, final QueryResponse rsp) {
- // TODO: blocked by SOLR-9309 (once this can be fixed, update jdocs)
- if (null != rsp.getResults()) { // TODO: replace this..
- // if (expectList) { // TODO: ...with this tighter check.
+ if (expectList) {
return rsp.getResults();
}