dsmiley commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1117980789
##########
solr/core/src/test/org/apache/solr/TestSolrCoreProperties.java:
##########
@@ -93,7 +91,7 @@ public void testSimple() throws Exception {
params(
"q", "*:*",
"echoParams", "all");
- QueryResponse res = getSolrClient().query(params);
+ QueryResponse res =
solrClientTestRule.getSolrClient(TestSolrCoreProperties.this).query(params);
assertEquals(0, res.getResults().getNumFound());
Review Comment:
Can't we keep this as it was? Don't want to update tests unnecessarily.
##########
solr/core/src/test/org/apache/solr/request/TestStreamBody.java:
##########
@@ -61,19 +62,16 @@ public void before() throws Exception {
if (random().nextBoolean()) {
log.info("These tests are run with V2 API");
restTestHarness.setServerProvider(
- () -> jetty.getBaseUrl().toString() + "/____v2/cores/" +
DEFAULT_TEST_CORENAME);
+ () -> getJetty().getBaseUrl().toString() + "/____v2/cores/" +
DEFAULT_TEST_CORENAME);
}
}
@After
public void after() throws Exception {
- if (jetty != null) {
- jetty.stop();
- jetty = null;
- }
- if (client != null) {
- client.close();
- client = null;
+ solrClientTestRule.reset();
+ if (SolrJettyTestRule.getClient() != null) {
+ SolrJettyTestRule.getClient().close();
+ SolrJettyTestRule.client = null;
Review Comment:
_Palm-to-face_! My suspicion is that you did a refactoring initiated by
your IDE and didn't check its results.
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.util;
+
+import static org.apache.solr.SolrTestCaseJ4.DEFAULT_TEST_CORENAME;
+import static org.apache.solr.SolrTestCaseJ4.getHttpSolrClient;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Path;
+import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.embedded.JettyConfig;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SolrJettyTestRule extends SolrClientTestRule {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private SolrClient client = null;
+ private JettySolrRunner jetty;
+ private SolrClient adminClient = null;
+
+ private ConcurrentHashMap<String, SolrClient> clients =
+ new ConcurrentHashMap<>(); // TODO close them
+
+ public SolrClient getClient() {
+ return client;
+ }
+
+ @Override
+ protected void after() {
+ if (adminClient != null) {
+ try {
+ adminClient.close();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ adminClient = null;
+
+ if (jetty != null) {
+
+ try {
+ jetty.stop();
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ jetty = null;
+ }
+
+ if (client != null) {
+ try {
+ client.close();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ client = null;
+ }
+ }
+
+ @Deprecated // Prefer not to have this.
+ public void reset() {
+ after();
+ }
+
+ @Override
+ public void startSolr(Path solrHome) {
+ startSolr(
+ solrHome,
+ new Properties(),
+ JettyConfig.builder()
+ .withSSLConfig(SolrTestCaseJ4.sslConfig.buildServerSSLConfig())
+ .build());
+ }
+
+ public void startSolr(Path solrHome, Properties nodeProperties, JettyConfig
jettyConfig) {
+
Review Comment:
Let's check "jetty" isn't already set? Would be nasty if this were to
happen accidentally.
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -52,13 +53,18 @@ protected void after() {
try {
jetty.stop();
} catch (Exception e) {
- throw new RuntimeException(e);
+ try {
+ throw new IOException(e);
+ } catch (IOException ex) {
+ throw new RuntimeException(ex);
+ }
}
jetty = null;
}
}
- public void reset() throws Exception {
+ @Deprecated // Prefer not to have this.
Review Comment:
It communicates we have a method that we don't really love; debatable. If
we had the time to tend to each and every subclass of SolrJettyTestBase, I
think we wouldn't have needed this. A comment would be helpful Joshua.
##########
solr/core/src/test/org/apache/solr/core/TestConfigSetImmutable.java:
##########
@@ -65,11 +66,8 @@ public void before() throws Exception {
@After
public void after() throws Exception {
- if (jetty != null) {
- jetty.stop();
- jetty = null;
- }
- client = null;
+ solrClientTestRule.reset();
+ SolrJettyTestRule.client = null;
Review Comment:
That's evil; breaks object-oriented principles. client should be private.
If this `client = null` line seems to be needed then apparently reset() isn't
doing its complete job.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientConPoolTest.java:
##########
@@ -48,12 +48,12 @@ public class HttpSolrClientConPoolTest extends
SolrJettyTestBase {
public static void beforeTest() throws Exception {
createAndStartJetty(legacyExampleCollection1SolrHome());
// stealing the first made jetty
- yetty = jetty;
+ yetty = getJetty();
barUrl = yetty.getBaseUrl().toString() + "/" + "collection1";
createAndStartJetty(legacyExampleCollection1SolrHome());
- fooUrl = jetty.getBaseUrl().toString() + "/" + "collection1";
+ fooUrl = getJetty().getBaseUrl().toString() + "/" + "collection1";
Review Comment:
I see this **so** often that it deserves a convenience method in
SolrJettyTestBase and also on the rule.
##########
solr/core/src/test/org/apache/solr/request/TestStreamBody.java:
##########
@@ -124,10 +122,11 @@ public String getPath() { // don't let superclass
substitute qt for the path
}
};
SolrException se =
- expectThrows(SolrException.class, () ->
queryRequest.process(getSolrClient()));
+ expectThrows(
+ SolrException.class, () ->
queryRequest.process(solrClientTestRule.getSolrClient()));
Review Comment:
Can't we keep this as it was? Don't want to update tests unnecessarily.
(as with other comments, I won't repeat if for each case of the same issue).
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -36,24 +36,22 @@
import org.apache.solr.embedded.JettySolrRunner;
import org.apache.solr.util.DirectoryUtil;
import org.apache.solr.util.ExternalPaths;
+import org.apache.solr.util.SolrJettyTestRule;
import org.eclipse.jetty.servlet.ServletHolder;
-import org.junit.After;
import org.junit.AfterClass;
import org.junit.BeforeClass;
+import org.junit.ClassRule;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public abstract class SolrJettyTestBase extends SolrTestCaseJ4 {
+ @ClassRule public static SolrJettyTestRule solrClientTestRule = new
SolrJettyTestRule();
+ public static String context;
Review Comment:
I'm suspicious we need context.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/response/TestSuggesterResponse.java:
##########
@@ -130,10 +131,10 @@ private void addSampleDocs() throws SolrServerException,
IOException {
SolrInputDocument doc3 = new SolrInputDocument();
doc3.setField("id", "333");
doc3.setField(field, "Laptop");
- client.add(doc);
- client.add(doc2);
- client.add(doc3);
- client.commit(true, true);
+ SolrJettyTestRule.getClient().add(doc);
Review Comment:
eh... Why would SolrJettyTestRule have a static getClient method? We've
spoken about static state (client is the state in this case) being usually
wrong.
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.util;
+
+import static org.apache.solr.SolrTestCaseJ4.DEFAULT_TEST_CORENAME;
+import static org.apache.solr.SolrTestCaseJ4.getHttpSolrClient;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Path;
+import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.embedded.JettyConfig;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SolrJettyTestRule extends SolrClientTestRule {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private SolrClient client = null;
+ private JettySolrRunner jetty;
+ private SolrClient adminClient = null;
+
+ private ConcurrentHashMap<String, SolrClient> clients =
+ new ConcurrentHashMap<>(); // TODO close them
+
+ public SolrClient getClient() {
+ return client;
+ }
+
+ @Override
+ protected void after() {
+ if (adminClient != null) {
+ try {
+ adminClient.close();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ adminClient = null;
+
+ if (jetty != null) {
+
+ try {
+ jetty.stop();
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ jetty = null;
+ }
+
+ if (client != null) {
+ try {
+ client.close();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ client = null;
+ }
+ }
+
+ @Deprecated // Prefer not to have this.
+ public void reset() {
+ after();
+ }
+
+ @Override
+ public void startSolr(Path solrHome) {
+ startSolr(
+ solrHome,
+ new Properties(),
+ JettyConfig.builder()
+ .withSSLConfig(SolrTestCaseJ4.sslConfig.buildServerSSLConfig())
+ .build());
+ }
+
+ public void startSolr(Path solrHome, Properties nodeProperties, JettyConfig
jettyConfig) {
+
+ jetty = new JettySolrRunner(solrHome.toString(), nodeProperties,
jettyConfig);
+ try {
+ jetty.start();
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
Review Comment:
Finally; you figured it out :-)
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.util;
+
+import static org.apache.solr.SolrTestCaseJ4.DEFAULT_TEST_CORENAME;
+import static org.apache.solr.SolrTestCaseJ4.getHttpSolrClient;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Path;
+import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.embedded.JettyConfig;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SolrJettyTestRule extends SolrClientTestRule {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private SolrClient client = null;
Review Comment:
There will no longer be one client but a map of clients keyed by collection
name.
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.util;
+
+import static org.apache.solr.SolrTestCaseJ4.DEFAULT_TEST_CORENAME;
+import static org.apache.solr.SolrTestCaseJ4.getHttpSolrClient;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Path;
+import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.embedded.JettyConfig;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SolrJettyTestRule extends SolrClientTestRule {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private SolrClient client = null;
+ private JettySolrRunner jetty;
+ private SolrClient adminClient = null;
+
+ private ConcurrentHashMap<String, SolrClient> clients =
+ new ConcurrentHashMap<>(); // TODO close them
+
+ public SolrClient getClient() {
+ return client;
+ }
+
+ @Override
+ protected void after() {
+ if (adminClient != null) {
+ try {
+ adminClient.close();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ adminClient = null;
+
+ if (jetty != null) {
+
+ try {
+ jetty.stop();
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ jetty = null;
+ }
+
+ if (client != null) {
+ try {
+ client.close();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ client = null;
+ }
+ }
+
+ @Deprecated // Prefer not to have this.
+ public void reset() {
+ after();
+ }
+
+ @Override
+ public void startSolr(Path solrHome) {
+ startSolr(
+ solrHome,
+ new Properties(),
+ JettyConfig.builder()
+ .withSSLConfig(SolrTestCaseJ4.sslConfig.buildServerSSLConfig())
+ .build());
+ }
+
+ public void startSolr(Path solrHome, Properties nodeProperties, JettyConfig
jettyConfig) {
+
+ jetty = new JettySolrRunner(solrHome.toString(), nodeProperties,
jettyConfig);
+ try {
+ jetty.start();
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ int port = jetty.getLocalPort();
+ log.info("Jetty Assigned Port#{}", port);
+ adminClient = getHttpSolrClient(jetty.getBaseUrl().toString());
+ }
+
+ public JettySolrRunner getJetty() {
+ if (jetty == null) throw new IllegalStateException("Jetty has not
started");
+ return jetty;
+ }
+
+ @Override
+ public SolrClient getSolrClient(String name) {
+ if (clients.containsKey(name)) {
+ return clients.get(name);
+ } else {
+ clients.put(
Review Comment:
see computeIfAbsent or similar
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.util;
+
+import static org.apache.solr.SolrTestCaseJ4.DEFAULT_TEST_CORENAME;
+import static org.apache.solr.SolrTestCaseJ4.getHttpSolrClient;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Path;
+import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.embedded.JettyConfig;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SolrJettyTestRule extends SolrClientTestRule {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private SolrClient client = null;
+ private JettySolrRunner jetty;
+ private SolrClient adminClient = null;
+
+ private ConcurrentHashMap<String, SolrClient> clients =
+ new ConcurrentHashMap<>(); // TODO close them
+
+ public SolrClient getClient() {
+ return client;
+ }
+
+ @Override
+ protected void after() {
+ if (adminClient != null) {
+ try {
+ adminClient.close();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ adminClient = null;
+
+ if (jetty != null) {
+
+ try {
+ jetty.stop();
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ jetty = null;
+ }
+
+ if (client != null) {
+ try {
+ client.close();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ client = null;
+ }
+ }
+
+ @Deprecated // Prefer not to have this.
+ public void reset() {
+ after();
+ }
+
+ @Override
+ public void startSolr(Path solrHome) {
+ startSolr(
+ solrHome,
+ new Properties(),
+ JettyConfig.builder()
+ .withSSLConfig(SolrTestCaseJ4.sslConfig.buildServerSSLConfig())
+ .build());
+ }
+
+ public void startSolr(Path solrHome, Properties nodeProperties, JettyConfig
jettyConfig) {
+
+ jetty = new JettySolrRunner(solrHome.toString(), nodeProperties,
jettyConfig);
+ try {
+ jetty.start();
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ int port = jetty.getLocalPort();
+ log.info("Jetty Assigned Port#{}", port);
+ adminClient = getHttpSolrClient(jetty.getBaseUrl().toString());
+ }
+
+ public JettySolrRunner getJetty() {
+ if (jetty == null) throw new IllegalStateException("Jetty has not
started");
+ return jetty;
+ }
+
+ @Override
+ public SolrClient getSolrClient(String name) {
+ if (clients.containsKey(name)) {
+ return clients.get(name);
+ } else {
+ clients.put(
+ name, getHttpSolrClient(jetty.getBaseUrl().toString() + "/" +
DEFAULT_TEST_CORENAME));
+ return getHttpSolrClient(jetty.getBaseUrl().toString() + "/" +
DEFAULT_TEST_CORENAME);
Review Comment:
Why do you create two clients in this method? Surely not correct.
##########
solr/core/src/test/org/apache/solr/TestTolerantSearch.java:
##########
@@ -104,10 +104,7 @@ public static void destroyThings() throws Exception {
collection2.close();
collection2 = null;
}
- if (null != jetty) {
- jetty.stop();
- jetty = null;
- }
+ solrClientTestRule.reset();
Review Comment:
True. But some tests like this one have a different lifecycle for Jetty
than the majority of SolrJettyTestBase subclasses, so got to do something.
Ideally we'd have time to tend to each test's needs, and then perhaps this test
wouldn't need the sort of explicit reset/close you see.
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -136,7 +136,7 @@ public synchronized SolrClient getSolrClient() {
* options.
*/
public SolrClient createNewSolrClient() {
Review Comment:
I agree with Eric; we should delete it.
The fact that it's overridden is a problem because the Rule is responsible
for creating the client. If there are tests that need to tweak the client then
those specific tests need to create the client and eventually close it as well.
It appears the override is in subclasses of SolrExampleTestsBase so you could
tend to this in one spot instead of each of its many subclasses.
For **new** tests that aren't subclassing SolrJettyTestBase, we could make
the method in this rule that creates these clients be overridable so that the
test can override via an anonymous inner class implementation. Or add some
general Builder transformer lambda thing (UnaryOperator). Not sure on the best
approach there; a TODO for the future.
--
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
