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: [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]
