You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "joshgog (via GitHub)" <gi...@apache.org> on 2023/02/21 11:06:39 UTC
[GitHub] [solr] joshgog opened a new pull request, #1374: SolrClientTestRule for JettySolrRunner
joshgog opened a new pull request, #1374:
URL: https://github.com/apache/solr/pull/1374
https://issues.apache.org/jira/browse/SOLR-16623
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
This PR introduces a SolrClientTestRule for JettySolrRunner
# Solution
Introduced SolrClientTestRule abstract class that extends JUnit's ExternalResource class, which is a TestRule that allows for specifying methods to be run before and after a test. SolrJettyTestRule is a concrete implementation of the abstract class and is declared as a ClassRule field in SolrJettyTestBase.
# Tests
The change is more of code refactoring. New tests are not required but the existing tests should pass
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1306315310
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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 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.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.common.util.IOUtils;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.embedded.JettyConfig;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A {@link SolrClientTestRule} that provides a Solr instance running in Jetty, an HTTP server. It's
+ * based off of {@link JettySolrRunner}.
+ */
+public class SolrJettyTestRule extends SolrClientTestRule {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private JettySolrRunner jetty;
+
+ private final ConcurrentHashMap<String, SolrClient> clients = new ConcurrentHashMap<>();
+ private boolean enableProxy;
+
+ @Override
+ protected void after() {
+ for (SolrClient solrClient : clients.values()) {
+ IOUtils.closeQuietly(solrClient);
+ }
+ clients.clear();
+
+ if (jetty != null) {
+ try {
+ jetty.stop();
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ jetty = null;
+ enableProxy = false;
+ }
+ }
+
+ /** Resets the state. DEPRECATED; please don't call! */
+ @Deprecated
+ public void reset() {
+ after();
+ }
+
+ @Override
+ public void startSolr(Path solrHome) {
+ startSolr(
+ solrHome,
+ new Properties(),
+ JettyConfig.builder()
+ .withSSLConfig(SolrTestCaseJ4.sslConfig.buildServerSSLConfig())
+ .build());
+ }
+
+ /** Enables proxy feature to disable connections. Must be called prior to starting. */
Review Comment:
I think the following does a slightly better job
> Enables proxy feature to allow for failure injection testing at the inter-node communication level
please rewrite as needed :)
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] joshgog commented on pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "joshgog (via GitHub)" <gi...@apache.org>.
joshgog commented on PR #1374:
URL: https://github.com/apache/solr/pull/1374#issuecomment-1460128836
> I don't recommend adding `// nocommit: Fails` type comments. The CI results tells you what fails, which can change as you improve things. In fact you just made a critical improvement and it seems there are only a few failures left. How's it going with them?
Still working on them
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1306313084
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientProxyTest.java:
##########
Review Comment:
looks good! I like this approach and being the only proxy test as far as I can see, I have to say thank you for adding support for it :)
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] joshgog commented on a diff in pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "joshgog (via GitHub)" <gi...@apache.org>.
joshgog commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1114796900
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -136,7 +136,7 @@ public synchronized SolrClient getSolrClient() {
* options.
*/
public SolrClient createNewSolrClient() {
Review Comment:
Its overridden by several subclasses e.g when creating a client `withConnectionTimeout` `withRequestWrite`
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1118128772
##########
solr/solrj/src/test/org/apache/solr/client/solrj/response/TestSuggesterResponse.java:
##########
@@ -119,8 +119,8 @@ public void testEmptySuggesterResponse() throws Exception {
}
private void addSampleDocs() throws SolrServerException, IOException {
- getClient().deleteByQuery("*:*");
- getClient().commit(true, true);
+ solrClientTestRule.getSolrClient().deleteByQuery("*:*");
+ solrClientTestRule.getSolrClient().commit(true, true);
Review Comment:
Nope; we agreed we'd leave much test code as it was, simply calling `getSolrClient` defined by SolrJettyTestBase which will indeed get the client from the rule.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] joshgog commented on a diff in pull request #1374: SolrClientTestRule for JettySolrRunner
Posted by "joshgog (via GitHub)" <gi...@apache.org>.
joshgog commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1113436405
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.lucene.tests.util.LuceneTestCase.createTempDir;
+import static org.apache.solr.SolrTestCaseJ4.DEFAULT_TEST_CORENAME;
+import static org.apache.solr.SolrTestCaseJ4.initCore;
+import static org.apache.solr.SolrTestCaseJ4.writeCoreProperties;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Path;
+import java.util.Properties;
+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 JettySolrRunner jetty;
+ private SolrClient adminClient = null;
+
+ @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 (Exception e) {
+ throw new RuntimeException(e);
+ }
+ jetty = null;
+ }
+ }
+
+ public void reset() throws Exception {
+ after();
+ }
+
+ @Override
+ public void startSolr(Path solrHome) {
+ startSolr(
+ solrHome,
+ new Properties(),
+ JettyConfig.builder()
+ .withSSLConfig(SolrTestCaseJ4.sslConfig.buildServerSSLConfig())
+ .build());
+ }
+
+ public void startSolr(Path solrHome, JettyConfig jettyConfig) {
+ startSolr(solrHome, new Properties(), jettyConfig);
+ }
+
+ public void startSolr(Path solrHome, Properties nodeProperties, JettyConfig jettyConfig) {
+
+ try {
+ initCore(null, null, solrHome.toString());
+
+ Path coresDir = createTempDir().resolve("cores");
+
+ Properties props = new Properties();
+ props.setProperty("name", DEFAULT_TEST_CORENAME);
+ props.setProperty("configSet", "collection1");
+ props.setProperty("config", "${solrconfig:solrconfig.xml}");
+ props.setProperty("schema", "${schema:schema.xml}");
+
+ writeCoreProperties(coresDir.resolve("core"), props, "RestTestBase");
+
+ Properties nodeProps = new Properties(nodeProperties);
+ nodeProps.setProperty("coreRootDirectory", coresDir.toString());
+ nodeProps.setProperty("configSetBaseDir", solrHome.toString());
+
+ jetty = new JettySolrRunner(solrHome.toString(), nodeProps, jettyConfig);
+ jetty.start();
+ int port = jetty.getLocalPort();
+ log.info("Jetty Assigned Port#{}", port);
+ adminClient = SolrTestCaseJ4.getHttpSolrClient(jetty.getBaseUrl().toString());
+ } catch (Exception e) {
+ log.error(e.toString());
+ }
+ }
+
+ public JettySolrRunner getJetty() {
+ if (jetty == null) throw new IllegalStateException("Jetty has not started");
+ return jetty;
+ }
+
+ @Override
+ public SolrClient getSolrClient(String name) {
+ throw new UnsupportedOperationException(); // TODO Client needed
Review Comment:
Its now implemented.` return getHttpSolrClient(jetty.getBaseUrl().toString() + "/" + DEFAULT_TEST_CORENAME);`
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] joshgog commented on pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "joshgog (via GitHub)" <gi...@apache.org>.
joshgog commented on PR #1374:
URL: https://github.com/apache/solr/pull/1374#issuecomment-1460090475
> I was going to mention that your "admin client" thing could be removed in lieu of a simple getSolrClient("") (empty string) with special handling? I think this would simplify the code / lifecycle. WDYT?
Sure will work on that
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] joshgog commented on a diff in pull request #1374: SolrClientTestRule for JettySolrRunner
Posted by "joshgog (via GitHub)" <gi...@apache.org>.
joshgog commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1113434652
##########
solr/core/src/test/org/apache/solr/TestSolrCoreProperties.java:
##########
@@ -79,11 +79,10 @@ public static void beforeTest() throws Exception {
if (System.getProperty("solr.data.dir") == null) {
nodeProperties.setProperty("solr.data.dir", createTempDir().toFile().getCanonicalPath());
}
- jetty =
- new JettySolrRunner(homeDir.getAbsolutePath(), nodeProperties, buildJettyConfig("/solr"));
- jetty.start();
- port = jetty.getLocalPort();
+ new JettySolrRunner(homeDir.getAbsolutePath(), nodeProperties, buildJettyConfig("/solr"));
Review Comment:
Right! He was supposed to do this `solrClientTestRule.startSolr(Path.of(homeDir.getAbsolutePath()), nodeProperties, buildJettyConfig("/solr"));`
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1374: SolrClientTestRule for JettySolrRunner
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1113462547
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -125,7 +125,7 @@ public static void afterSolrJettyTestBase() throws Exception {
public synchronized SolrClient getSolrClient() {
if (client == null) {
- client = createNewSolrClient();
+ client = solrClientTestRule.getSolrClient();
Review Comment:
I LOVE this! So many "createXYZClient" littering the test code!
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1374:
URL: https://github.com/apache/solr/pull/1374#issuecomment-1440449203
FYI Eric/others this is a re-do PR to reduce the scope of change to JettySolrTestBase & subclasses. Too much work to fix/improve them in greater detail.
From our online review today:
* Exception handling is wrong; details were discussed
* SolrClientTestRule.getSolrClient (and any overloads) should be documented to say "The caller doesn't need to close it." And furthermore, the method _may_ return the same instance for the same collection.
* SolrJettyTestBase shouldn't have a field for the SolrClient, just as it doesn't have a field for Jetty either (thanks to your change). The client's lifecycle (create & close) should be managed by the rule.
* In the rule (perhaps even in SolrClientTestRule!), add a ConcurrentHashMap to track the clients by their collection name. Thankfully (due to Eric Pugh's heroism), clients are mostly immutable thus we can return the same instance. Obviously they should be closed out in `after()`.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1374: SolrClientTestRule for JettySolrRunner
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1113080359
##########
solr/core/src/test/org/apache/solr/TestSolrCoreProperties.java:
##########
@@ -79,11 +79,10 @@ public static void beforeTest() throws Exception {
if (System.getProperty("solr.data.dir") == null) {
nodeProperties.setProperty("solr.data.dir", createTempDir().toFile().getCanonicalPath());
}
- jetty =
- new JettySolrRunner(homeDir.getAbsolutePath(), nodeProperties, buildJettyConfig("/solr"));
- jetty.start();
- port = jetty.getLocalPort();
+ new JettySolrRunner(homeDir.getAbsolutePath(), nodeProperties, buildJettyConfig("/solr"));
Review Comment:
does this guy need to be set to something?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] joshgog commented on pull request #1374: SolrClientTestRule for JettySolrRunner
Posted by "joshgog (via GitHub)" <gi...@apache.org>.
joshgog commented on PR #1374:
URL: https://github.com/apache/solr/pull/1374#issuecomment-1438939673
The reset() method is not desired to be in the rule that's why its annotated with ` @Deprecated // Prefer not to have this.`
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1374: SolrClientTestRule for JettySolrRunner
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1113461724
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.lucene.tests.util.LuceneTestCase.createTempDir;
+import static org.apache.solr.SolrTestCaseJ4.DEFAULT_TEST_CORENAME;
+import static org.apache.solr.SolrTestCaseJ4.initCore;
+import static org.apache.solr.SolrTestCaseJ4.writeCoreProperties;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Path;
+import java.util.Properties;
+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 JettySolrRunner jetty;
+ private SolrClient adminClient = null;
+
+ @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 (Exception e) {
+ throw new RuntimeException(e);
+ }
+ jetty = null;
+ }
+ }
+
+ public void reset() throws Exception {
+ after();
+ }
+
+ @Override
+ public void startSolr(Path solrHome) {
+ startSolr(
+ solrHome,
+ new Properties(),
+ JettyConfig.builder()
+ .withSSLConfig(SolrTestCaseJ4.sslConfig.buildServerSSLConfig())
+ .build());
+ }
+
+ public void startSolr(Path solrHome, JettyConfig jettyConfig) {
+ startSolr(solrHome, new Properties(), jettyConfig);
+ }
+
+ public void startSolr(Path solrHome, Properties nodeProperties, JettyConfig jettyConfig) {
+
+ try {
+ initCore(null, null, solrHome.toString());
+
+ Path coresDir = createTempDir().resolve("cores");
+
+ Properties props = new Properties();
+ props.setProperty("name", DEFAULT_TEST_CORENAME);
+ props.setProperty("configSet", "collection1");
+ props.setProperty("config", "${solrconfig:solrconfig.xml}");
+ props.setProperty("schema", "${schema:schema.xml}");
+
+ writeCoreProperties(coresDir.resolve("core"), props, "RestTestBase");
+
+ Properties nodeProps = new Properties(nodeProperties);
+ nodeProps.setProperty("coreRootDirectory", coresDir.toString());
+ nodeProps.setProperty("configSetBaseDir", solrHome.toString());
+
+ jetty = new JettySolrRunner(solrHome.toString(), nodeProps, jettyConfig);
+ jetty.start();
+ int port = jetty.getLocalPort();
+ log.info("Jetty Assigned Port#{}", port);
+ adminClient = SolrTestCaseJ4.getHttpSolrClient(jetty.getBaseUrl().toString());
+ } catch (Exception e) {
+ log.error(e.toString());
+ }
+ }
+
+ public JettySolrRunner getJetty() {
+ if (jetty == null) throw new IllegalStateException("Jetty has not started");
+ return jetty;
+ }
+
+ @Override
+ public SolrClient getSolrClient(String name) {
+ throw new UnsupportedOperationException(); // TODO Client needed
Review Comment:
awesome!
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] joshgog commented on a diff in pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "joshgog (via GitHub)" <gi...@apache.org>.
joshgog commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1114780615
##########
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 not part of the scope of this PR.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] Walton-guud-Kenya commented on pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "Walton-guud-Kenya (via GitHub)" <gi...@apache.org>.
Walton-guud-Kenya commented on PR #1374:
URL: https://github.com/apache/solr/pull/1374#issuecomment-1460084420
>
Still working on the failing tests
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1306288103
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientProxyTest.java:
##########
Review Comment:
@stillalex please take a look at my changes here and let me know if you think they are fine, or if you have any comments in general
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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 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.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.common.util.IOUtils;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.embedded.JettyConfig;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A {@link SolrClientTestRule} that provides a Solr instance running in Jetty, an HTTP server. It's
+ * based off of {@link JettySolrRunner}.
+ */
+public class SolrJettyTestRule extends SolrClientTestRule {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private JettySolrRunner jetty;
+
+ private final ConcurrentHashMap<String, SolrClient> clients = new ConcurrentHashMap<>();
+ private boolean enableProxy;
+
+ @Override
+ protected void after() {
+ for (SolrClient solrClient : clients.values()) {
+ IOUtils.closeQuietly(solrClient);
+ }
+ clients.clear();
+
+ if (jetty != null) {
+ try {
+ jetty.stop();
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ jetty = null;
+ enableProxy = false;
+ }
+ }
+
+ /** Resets the state. DEPRECATED; please don't call! */
+ @Deprecated
+ public void reset() {
+ after();
+ }
+
+ @Override
+ public void startSolr(Path solrHome) {
+ startSolr(
+ solrHome,
+ new Properties(),
+ JettyConfig.builder()
+ .withSSLConfig(SolrTestCaseJ4.sslConfig.buildServerSSLConfig())
+ .build());
+ }
+
+ /** Enables proxy feature to disable connections. Must be called prior to starting. */
Review Comment:
I copied the first sentence from a comment in JettySolrRunner written by Mark Miller but honestly I don't understand it. @stillalex maybe you could recommend better info as you just recently used it?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1374:
URL: https://github.com/apache/solr/pull/1374#issuecomment-1458877554
I was going to mention that your "admin client" thing could be removed in lieu of a simple getSolrClient("") (empty string) with special handling? I think this would simplify the code / lifecycle. WDYT?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1374:
URL: https://github.com/apache/solr/pull/1374#issuecomment-1458876462
I don't recommend adding `// nocommit: Fails` type comments. The CI results tells you what fails, which can change as you improve things. In fact you just made a critical improvement and it seems there are only a few failures left. How's it going with them?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1374: SolrClientTestRule for JettySolrRunner
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1113146801
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.lucene.tests.util.LuceneTestCase.createTempDir;
+import static org.apache.solr.SolrTestCaseJ4.DEFAULT_TEST_CORENAME;
+import static org.apache.solr.SolrTestCaseJ4.initCore;
+import static org.apache.solr.SolrTestCaseJ4.writeCoreProperties;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Path;
+import java.util.Properties;
+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 JettySolrRunner jetty;
+ private SolrClient adminClient = null;
+
+ @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 (Exception e) {
+ throw new RuntimeException(e);
+ }
+ jetty = null;
+ }
+ }
+
+ public void reset() throws Exception {
+ after();
+ }
+
+ @Override
+ public void startSolr(Path solrHome) {
+ startSolr(
+ solrHome,
+ new Properties(),
+ JettyConfig.builder()
+ .withSSLConfig(SolrTestCaseJ4.sslConfig.buildServerSSLConfig())
+ .build());
+ }
+
+ public void startSolr(Path solrHome, JettyConfig jettyConfig) {
+ startSolr(solrHome, new Properties(), jettyConfig);
+ }
+
+ public void startSolr(Path solrHome, Properties nodeProperties, JettyConfig jettyConfig) {
+
+ try {
+ initCore(null, null, solrHome.toString());
+
+ Path coresDir = createTempDir().resolve("cores");
+
+ Properties props = new Properties();
+ props.setProperty("name", DEFAULT_TEST_CORENAME);
+ props.setProperty("configSet", "collection1");
+ props.setProperty("config", "${solrconfig:solrconfig.xml}");
+ props.setProperty("schema", "${schema:schema.xml}");
+
+ writeCoreProperties(coresDir.resolve("core"), props, "RestTestBase");
+
+ Properties nodeProps = new Properties(nodeProperties);
+ nodeProps.setProperty("coreRootDirectory", coresDir.toString());
+ nodeProps.setProperty("configSetBaseDir", solrHome.toString());
+
+ jetty = new JettySolrRunner(solrHome.toString(), nodeProps, jettyConfig);
+ jetty.start();
+ int port = jetty.getLocalPort();
+ log.info("Jetty Assigned Port#{}", port);
+ adminClient = SolrTestCaseJ4.getHttpSolrClient(jetty.getBaseUrl().toString());
+ } catch (Exception e) {
+ log.error(e.toString());
+ }
+ }
+
+ public JettySolrRunner getJetty() {
+ if (jetty == null) throw new IllegalStateException("Jetty has not started");
+ return jetty;
+ }
+
+ @Override
+ public SolrClient getSolrClient(String name) {
+ throw new UnsupportedOperationException(); // TODO Client needed
Review Comment:
is this still a todo?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1374: SolrClientTestRule for JettySolrRunner
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1113085635
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.lucene.tests.util.LuceneTestCase.createTempDir;
+import static org.apache.solr.SolrTestCaseJ4.DEFAULT_TEST_CORENAME;
+import static org.apache.solr.SolrTestCaseJ4.initCore;
+import static org.apache.solr.SolrTestCaseJ4.writeCoreProperties;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Path;
+import java.util.Properties;
+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 {
Review Comment:
Can we get some javadocs on how to use this? I get that it's a SolrJettyTestRule, but can you provide some docson when I would use it? What are good use cases for using this rule....????
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1117988442
##########
solr/test-framework/src/java/org/apache/solr/util/SolrClientTestRule.java:
##########
@@ -135,14 +135,16 @@ protected void create(NewCollectionBuilder b) throws SolrServerException, IOExce
req.process(getAdminClient());
}
- /** Provides a SolrClient instance for administration actions */
+ /**
+ * Provides a SolrClient instance for administration actions. The caller doesn't need to close it
+ */
public abstract SolrClient getAdminClient();
- /** Provides a SolrClient instance for collection1 */
+ /** Provides a SolrClient instance for collection1. The caller doesn't need to close it */
public SolrClient getSolrClient() {
return getSolrClient("collection1");
}
-
+ /** Provides a SolrClient instance for provided name. The caller doesn't need to close it */
Review Comment:
"For provided name"? And what is this "name"?
##########
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:
Looking at the state of SolrJettyTestRule right now, it's already *not* static and is public -- which is good. Your PR doesn't compile. I suggest never pushing code that doesn't even compile.
##########
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
Review Comment:
That's a critical TODO by the way, and an easy one to get done.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1374: SolrClientTestRule for JettySolrRunner
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1374:
URL: https://github.com/apache/solr/pull/1374#issuecomment-1438599202
Really excited to see progress on anything that makes testing simpler! I hope my comments are being asked prematurely, if you aren't quite ready for review. I am somewhat trying to figure out how this makes our lives simpler in testing... (Partly me coming up the curve on TestRule's!)... For example, I expected to see many of the places where we shutdown Jetty to go completely away, as the TestRule would do it? I see a lot of `solrClientTestRule.reset();` however....
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1374: SolrClientTestRule for JettySolrRunner
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1113462935
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -136,7 +136,7 @@ public synchronized SolrClient getSolrClient() {
* options.
*/
public SolrClient createNewSolrClient() {
Review Comment:
can we just delete this method? Do we still need createNewSolrClient?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1374:
URL: https://github.com/apache/solr/pull/1374#issuecomment-1440473853
Thanks for sharing the summary from the review... I think you answered some questions I had! Really looking forward to seeing this land ;-).
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1374: SolrClientTestRule for JettySolrRunner
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1113065021
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -83,7 +83,8 @@ public static JettySolrRunner createAndStartJetty(
nodeProps.setProperty("solr.data.dir", createTempDir().toFile().getCanonicalPath());
}
- return createAndStartJetty(solrHome, nodeProps, jettyConfig);
+ solrClientTestRule.startSolr(Path.of(solrHome), nodeProps, jettyConfig);
Review Comment:
<picture><img alt="5% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/5/display.svg"></picture>
<b>*THREAD_SAFETY_VIOLATION:</b>* Unprotected write. Non-private method `SolrJettyTestBase.createAndStartJetty(...)` indirectly mutates container `util.ObjectReleaseTracker.OBJECTS` via call to `Map.remove(...)` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
ââ <b>2 similar findings have been found in this PR</b>
<details><summary>đ Expand here to view all instances of this finding</summary><br/>
<div align=\"center\">
| **File Path** | **Line Number** |
| ------------- | ------------- |
| solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java | [98](https://github.com/apache/solr/blob/0bfe49cee212cdc85c67bc2cb8840bf473b12eb5/solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java#L98) |
| solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java | [103](https://github.com/apache/solr/blob/0bfe49cee212cdc85c67bc2cb8840bf473b12eb5/solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java#L103) |
<p><a href="https://lift.sonatype.com/results/github.com/apache/solr/01GST04VC5DPP7NW9YAKB5D3AN?t=Infer|THREAD_SAFETY_VIOLATION" target="_blank">Visit the Lift Web Console</a> to find more details in your report.</p></div></details>
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=400643998&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=400643998&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=400643998&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=400643998&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=400643998&lift_comment_rating=5)â]
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] joshgog commented on a diff in pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "joshgog (via GitHub)" <gi...@apache.org>.
joshgog commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1118122343
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -136,7 +136,7 @@ public synchronized SolrClient getSolrClient() {
* options.
*/
public SolrClient createNewSolrClient() {
Review Comment:
Deleting it makes tests to fail with the error `java.lang.IllegalStateException: Connection pool shut down` I'm trying to crack the cause
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley merged pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #1374:
URL: https://github.com/apache/solr/pull/1374
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1374:
URL: https://github.com/apache/solr/pull/1374#issuecomment-1666952512
Some nice cleans up @dsmiley in the current round! The tests read much simpler...
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1285847169
##########
solr/test-framework/src/java/org/apache/solr/util/SolrJettyTestRule.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.lucene.tests.util.LuceneTestCase.createTempDir;
+import static org.apache.solr.SolrTestCaseJ4.DEFAULT_TEST_CORENAME;
+import static org.apache.solr.SolrTestCaseJ4.initCore;
+import static org.apache.solr.SolrTestCaseJ4.writeCoreProperties;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Path;
+import java.util.Properties;
+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 {
Review Comment:
I'd say that in general you should test with the most basic Solr possible based on what you are testing. If you can use EmbeddedSolrServerTestRule then do that. But if you need to test something relating to HTTP or Jetty then you're going to have to use this. Likewise, this isn't good enough for SolrCloud.
As a subclass of SolrClientTestRule, the hope is that it be familiar enough so that how to use it is self-explanatory provided you're familiar with the other SolrClientTestRule's (there is just one other right now).
If this is embraced everywhere that it should, fewer tests need to even be aware of JettySolrRunner. And maybe SolrJettyTestBase won't exist.
I'm maybe not directly answering your question but I'm hoping it's more self evident as it's used more and more. Initially, mostly it's just SolrJettyTestBase. Maybe more of SJTB should be folded into this rule.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1374: SolrClientTestRule for JettySolrRunner
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1113463405
##########
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:
How would we remove these? Is that part of the scope of this PR or it's own PR?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1374: SolrClientTestRule for JettySolrRunner
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1374:
URL: https://github.com/apache/solr/pull/1374#issuecomment-1438961097
Looking forward to this!
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1374: SolrClientTestRule for JettySolrRunner
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1374:
URL: https://github.com/apache/solr/pull/1374#discussion_r1113147827
##########
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:
i mentally thought that by using a test rule, some of this clean up stuff would just be handled by us, because it's a rule?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1374:
URL: https://github.com/apache/solr/pull/1374#issuecomment-1445406355
This is going to be great!
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1374: SOLR-16623:SolrClientTestRule for JettySolrRunnerV2
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org