dsmiley commented on code in PR #3418: URL: https://github.com/apache/solr/pull/3418#discussion_r2486972983
########## solr/core/src/java/org/apache/solr/search/combine/package-info.java: ########## Review Comment: Arguably, this package should be under org.apache.solr.handler.component, and then it would consolidate all the functionality added in this PR, not just these 2 classes. I know the combiner thing here is sort of general but it's only used by the new CombineQueryComponent, and I have no reason to believe that it's going to be used by new things that aren't search components. ########## solr/core/src/test/org/apache/solr/handler/component/CombinedQuerySolrCloudTest.java: ########## @@ -0,0 +1,245 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.handler.component; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import org.apache.solr.client.solrj.response.QueryResponse; +import org.apache.solr.cloud.AbstractFullDistribZkTestBase; +import org.apache.solr.common.SolrInputDocument; +import org.apache.solr.common.params.CommonParams; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Contains integration tests for the combined query functionality in a SolrCloud environment. This + * class extends AbstractFullDistribZkTestBase to leverage the distributed testing framework. The + * tests cover various scenarios including single and multiple lexical queries, sorting, pagination, + * faceting, and highlighting similar to {@link DistributedCombinedQueryComponentTest} + */ +public class CombinedQuerySolrCloudTest extends AbstractFullDistribZkTestBase { + + private static final int NUM_DOCS = 10; + private static final String vectorField = "vector"; + + @BeforeClass + public static void setUpClass() throws Exception { + initCore("solrconfig-combined-query.xml", "schema-vector-catchall.xml"); Review Comment: `initCore` should not be called for these `...Distrib...TestBase` test classes. Cores are managed by the base test infra. ########## 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 don't think extending BaseDistributedSearchTestCase makes sense in this scenario. BaseDistributedSearchTestCase compares a "control" client (single shard) to a distributed setup of a configurable number of shards; this is its fundamental purpose. Calling `fixShardCount(1)` misses the point. Can the single-sharded case can be tested with STCJ4? If not for some weird reason, surely SolrCloudTestCase. ########## solr/core/src/test/org/apache/solr/handler/component/DistributedCombinedQueryComponentTest.java: ########## @@ -0,0 +1,284 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.handler.component; + +import java.net.URI; +import java.net.URISyntaxException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import org.apache.solr.BaseDistributedSearchTestCase; +import org.apache.solr.client.solrj.impl.HttpSolrClient; +import org.apache.solr.client.solrj.response.QueryResponse; +import org.apache.solr.common.SolrInputDocument; +import org.apache.solr.common.params.CommonParams; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * The DistributedCombinedQueryComponentTest class is a JUnit test suite that evaluates the + * functionality of the CombinedQueryComponent in a Solr distributed search environment. It focuses + * on testing the integration of combiner queries with different configurations. + */ +public class DistributedCombinedQueryComponentTest extends BaseDistributedSearchTestCase { + + private static final int NUM_DOCS = 10; + private static final String vectorField = "vector"; + + /** + * Sets up the test class by initializing the core and setting system properties. This method is + * executed before all test methods in the class. + * + * @throws Exception if any exception occurs during initialization + */ + @BeforeClass + public static void setUpClass() throws Exception { + initCore("solrconfig-combined-query.xml", "schema-vector-catchall.xml"); Review Comment: `initCore` should not be called for these abstract distrib test classes. Cores are managed by the base test infra. You can set the config it should use via the static String field `configString` & `schemaString`. So ugly/hacky, I know! ########## solr/core/src/test/org/apache/solr/handler/component/CombinedQuerySolrCloudTest.java: ########## @@ -0,0 +1,245 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.handler.component; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import org.apache.solr.client.solrj.response.QueryResponse; +import org.apache.solr.cloud.AbstractFullDistribZkTestBase; +import org.apache.solr.common.SolrInputDocument; +import org.apache.solr.common.params.CommonParams; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Contains integration tests for the combined query functionality in a SolrCloud environment. This + * class extends AbstractFullDistribZkTestBase to leverage the distributed testing framework. The + * tests cover various scenarios including single and multiple lexical queries, sorting, pagination, + * faceting, and highlighting similar to {@link DistributedCombinedQueryComponentTest} + */ +public class CombinedQuerySolrCloudTest extends AbstractFullDistribZkTestBase { + + private static final int NUM_DOCS = 10; + private static final String vectorField = "vector"; + + @BeforeClass + public static void setUpClass() throws Exception { + initCore("solrconfig-combined-query.xml", "schema-vector-catchall.xml"); + System.setProperty("validateAfterInactivity", "200"); + System.setProperty("solr.httpclient.retries", "0"); + System.setProperty("distribUpdateSoTimeout", "5000"); + } + + @Override + protected String getCloudSolrConfig() { + return "solrconfig-combined-query.xml"; + } + + private synchronized void prepareIndexDocs() throws Exception { + List<SolrInputDocument> docs = new ArrayList<>(); + fixShardCount(2); Review Comment: should be called in `@BeforeClass` place -- 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]
