dsmiley commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1118019036
##########
solr/core/src/test/org/apache/solr/request/TestRemoteStreaming.java:
##########
@@ -116,7 +116,7 @@ public void testNoUrlAccess() throws Exception {
SolrQuery query = new SolrQuery();
query.setQuery("*:*"); // for anything
query.add("stream.url", makeDeleteAllUrl());
- try (SolrClient solrClient = createNewSolrClient()) {
+ try (SolrClient solrClient = getSolrClient()) {//TODO Close the client
Review Comment:
This TODO is already handled :-). If you don't recognize this, then learn
about Java's "try-with-resources" pattern.
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -138,17 +142,15 @@ public static void afterSolrJettyTestBase() throws
Exception {
solrClientTestRule.reset();
}
- /**
- * Create a new solr client. If createJetty was called, a http
implementation will be created,
- * otherwise an embedded implementation will be created. Subclasses should
override for other
- * options.
- */
- public SolrClient createNewSolrClient() {
- return solrClientTestRule.getSolrClient();
+ public synchronized SolrClient getSolrClient() {
+ if (getClient() == null) {
Review Comment:
Why would this ever return null? Why is it synchronized?
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -129,6 +129,10 @@ public static JettySolrRunner getJetty() {
return solrClientTestRule.getJetty();
}
+ public static SolrClient getClient() {
Review Comment:
I don't think we need to add new API methods to this class that didn't
already exist
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -31,18 +31,13 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+// TODO write javadocs
public class SolrJettyTestRule extends SolrClientTestRule {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- private SolrClient client = null;
private JettySolrRunner jetty;
+ private SolrClient client = null;
Review Comment:
Why is "client" its own field yet we've got this "clients" map?
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -75,9 +70,22 @@ protected void after() {
}
client = null;
}
+
+ for (SolrClient solrClient : clients.values()) {
Review Comment:
The sequence of actions in a lifecycle method (and this is one) can
sometimes matter. Do things in reverse order in a closing/after type of
method. Thus clients should be closed and *then* close Jetty. It's the
reverse order from starting.
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -138,17 +142,15 @@ public static void afterSolrJettyTestBase() throws
Exception {
solrClientTestRule.reset();
}
- /**
- * Create a new solr client. If createJetty was called, a http
implementation will be created,
- * otherwise an embedded implementation will be created. Subclasses should
override for other
- * options.
- */
- public SolrClient createNewSolrClient() {
- return solrClientTestRule.getSolrClient();
+ public synchronized SolrClient getSolrClient() {
+ if (getClient() == null) {
+ return solrClientTestRule.getSolrClient();
+ }
+ return getClient();
}
Review Comment:
When the method fundamentally changes away from using fields on this class
to calling the rule, it no longer needs to be synchronized because the rule's
getSolrClient is thread-safe.
also, don't see why getClient exists nor why it would return null.
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -94,33 +102,37 @@ public void startSolr(Path solrHome) {
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);
+ if (jetty == null) {
+
+ 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());
}
- 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;
}
+ public SolrClient getClient() {
Review Comment:
What's this for? Seems redundant with getSolrClient; no? If you had
documented it, maybe you would have self-discovered the duplicity
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -31,18 +31,13 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+// TODO write javadocs
Review Comment:
you will find a ton of TODOs across Solr but no "nocommit". "nocommit" is
checked for in the build and the PR is unmergeable until these are addressed.
##########
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:
I don't think you understood my meaning; admittedly what I said was
ambiguous. Let me put it like this, if someone calls startSolr yet startSolr
was already called, I believe this is quite erroneous! To *not* throw an
exception and pretend everything is fine could easily lead to strange behavior
to the caller (the test). Like maybe the startup parameters changed. But
fundamentally, the test is simply using this wrong.
--
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
