dsmiley commented on code in PR #3418:
URL: https://github.com/apache/solr/pull/3418#discussion_r2352636387
##########
solr/core/src/test/org/apache/solr/handler/component/CombinedQueryComponentTest.java:
##########
@@ -21,39 +21,59 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
-import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.BaseDistributedSearchTestCase;
+import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.common.util.SimpleOrderedMap;
import org.junit.BeforeClass;
import org.junit.Test;
/**
* The CombinedQueryComponentTest class is an integration test suite for the
CombinedQueryComponent
- * in Solr. It verifies the functionality of the component by performing few
basic queries and
- * validating the responses including limitations and combiner plugin.
+ * in Solr. It verifies the functionality of the component by performing few
basic queries in single
+ * sharded mode and validating the responses including limitations and
combiner plugin.
*/
-public class CombinedQueryComponentTest extends SolrTestCaseJ4 {
+public class CombinedQueryComponentTest extends BaseDistributedSearchTestCase {
Review Comment:
I'm now confused on how to differentiate the testing approach between this
class and DistributedCombinedQueryComponentTest. _That_ one is named
appropriately (consistently with other distributed tests) and extends
BaseDistributedSearchTestCase. I'm not sure *this* test has a role/purpose if
that one can be comprehensive.
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java:
##########
@@ -500,6 +505,28 @@ public void prepDistributed(ResponseBuilder rb) {
}
}
+ private static void forceDistributed(ResponseBuilder rb) {
+ SolrQueryRequest req = rb.req;
+ ModifiableSolrParams solrParams = new
ModifiableSolrParams(req.getParams());
+ solrParams.set("shortCircuit", false);
+ req.setParams(solrParams);
+ if (req.getHttpSolrCall() != null
+ && StringUtils.isEmpty(req.getParams().get(ShardParams.SHARDS))) {
+ String scheme = req.getHttpSolrCall().getReq().getScheme();
+ String host = req.getHttpSolrCall().getReq().getServerName();
+ int port = req.getHttpSolrCall().getReq().getServerPort();
+ String context = req.getHttpSolrCall().getReq().getContextPath();
+ String core = req.getCore().getName();
+ String localShardUrl =
+ String.format(Locale.ROOT, "%s://%s:%d%s/%s", scheme, host, port,
context, core);
+ solrParams.set(ShardParams.SHARDS, localShardUrl);
+ req.setParams(solrParams);
+ return;
+ }
Review Comment:
This part looks suspicious to me; so shortCircuit=false isn't enough? Did
you construct this based on seeing similar code elsewhere (where?) to create a
shards URL?
##########
solr/core/src/java/org/apache/solr/handler/component/ResponseBuilder.java:
##########
@@ -141,6 +141,15 @@ public ResponseBuilder(
public List<ShardRequest> outgoing; // requests to be sent
public List<ShardRequest> finished; // requests that have received responses
from all shards
public String shortCircuitedURL;
+ private boolean forcedDistrib = false;
Review Comment:
I thought of this as well, yet it feels weird to put a request aspect into
the response data holder. It doesn't affect the response. Any way... it's
really minor. This is pragmatic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
