dsmiley commented on code in PR #2625: URL: https://github.com/apache/solr/pull/2625#discussion_r1711432544
########## solr/core/src/test/org/apache/solr/TestCrossCoreJoin.java: ########## @@ -246,6 +261,7 @@ public String query(SolrCore core, SolrQueryRequest req) throws Exception { @AfterClass public static void nukeAll() { + systemClearPropertySolrTestsMergePolicyFactory(); Review Comment: Should not be necessary because our test infrastructure rules reset all system properties to as they are at the start. ########## solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java: ########## @@ -1324,6 +1335,16 @@ public static SolrQueryRequest req(SolrParams params, String... moreParams) { return new LocalSolrQueryRequest(h.getCore(), mp); } + /** Generates a SolrQueryRequest randomly assigning multi or single threaded processing. */ + public static SolrQueryRequest reqRandMulti(SolrParams params, String... moreParams) { Review Comment: I don't like this approach. Imagine that we embrace this -- would this then mean all/most calls to `req` would switch to `reqAndMulti`? Not good. Assuming we imagine that `multiThreaded=true` should just work (even if may not actually use multiple threads for various reasons), I'd prefer we change a system property to have true/false and randomly chosen. ########## solr/test-framework/src/java/org/apache/solr/util/TestHarness.java: ########## @@ -453,6 +455,22 @@ public LocalRequestFactory() {} */ @SuppressWarnings({"unchecked"}) public LocalSolrQueryRequest makeRequest(String... q) { + return makeRequest(false, q); + } + + /** + * Creates a LocalSolrQueryRequest based on variable args. Note that tests that want to test + * multi-threading should ensure that multiple commits are used and that the merge policy will + * leave a suitable number of segments. + * + * @param testMultiThreadedRandom if true 50% of requests will include the multiThreaded=true + * parameter. + * @param q the parameters for the request. + * @see #makeRequest(String...) + * @return a LocalSolrQueryRequest based on the supplied parameters + */ + @SuppressWarnings("unchecked") + public LocalSolrQueryRequest makeRequest(boolean testMultiThreadedRandom, String... q) { Review Comment: TestHarness might not yet be deprecated but I'd prefer we treat it as such. I should deprecate it. https://issues.apache.org/jira/browse/SOLR-11872 Even if SOLR-11872 fails to go anywhere, I also object to this new method here. ########## solr/core/src/test/org/apache/solr/search/join/GraphQueryTest.java: ########## @@ -19,16 +19,24 @@ import org.apache.solr.SolrTestCaseJ4; import org.apache.solr.common.SolrException; import org.apache.solr.common.params.SolrParams; +import org.apache.solr.index.NoMergePolicyFactory; +import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Test; public class GraphQueryTest extends SolrTestCaseJ4 { @BeforeClass public static void beforeTests() throws Exception { + systemSetPropertySolrTestsMergePolicyFactory(NoMergePolicyFactory.class.getName()); initCore("solrconfig.xml", "schema_latest.xml"); } + @AfterClass + public static void afterTests() { Review Comment: again, not needed ########## solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java: ########## @@ -1324,6 +1335,16 @@ public static SolrQueryRequest req(SolrParams params, String... moreParams) { return new LocalSolrQueryRequest(h.getCore(), mp); } + /** Generates a SolrQueryRequest randomly assigning multi or single threaded processing. */ + public static SolrQueryRequest reqRandMulti(SolrParams params, String... moreParams) { + ModifiableSolrParams mp = new ModifiableSolrParams(params); + for (int i = 0; i < moreParams.length; i += 2) { + mp.add(moreParams[i], moreParams[i + 1]); + } + mp.add("multiThreaded", String.valueOf(random().nextBoolean())); Review Comment: If we need to augment parameters, consider SolrParams.wrapDefaults. It'd be nice if we also had a SolrParams.of(name, value) to make it easy/idiomatic to combine. -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org