Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions solr/core/src/test/org/apache/solr/SolrTestCaseJ4Test.java
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@ public static void beforeClass() throws Exception {
PathUtils.touch(tmpSolrHome.resolve("core0/core.properties"));
PathUtils.touch(tmpSolrHome.resolve("core1/core.properties"));

Files.copy(getFile("solr/solr.xml"), tmpSolrHome.resolve("solr.xml"));

initCore("solrconfig-minimal.xml", "schema-tiny.xml", tmpSolrHome, "core1");
}

Expand Down
17 changes: 5 additions & 12 deletions solr/core/src/test/org/apache/solr/TestCustomCoreProperties.java
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,11 @@ public static void beforeClass() throws Exception {

Files.createDirectories(confDir);

String src_dir = TEST_HOME() + "/collection1/conf";
Files.copy(Path.of(src_dir, "schema-tiny.xml"), confDir.resolve("schema.xml"));
String srcDir = TEST_HOME() + "/collection1/conf";
Files.copy(Path.of(srcDir, "schema-tiny.xml"), confDir.resolve("schema.xml"));
Files.copy(Path.of(srcDir, "solrconfig-coreproperties.xml"), confDir.resolve("solrconfig.xml"));
Files.copy(
Path.of(src_dir, "solrconfig-coreproperties.xml"), confDir.resolve("solrconfig.xml"));
Files.copy(
Path.of(src_dir, "solrconfig.snippet.randomindexconfig.xml"),
Path.of(srcDir, "solrconfig.snippet.randomindexconfig.xml"),
confDir.resolve("solrconfig.snippet.randomindexconfig.xml"));

Properties p = new Properties();
Expand All @@ -72,13 +71,7 @@ public static void beforeClass() throws Exception {
coreProperties.store(fos, null);
}

Properties nodeProperties = new Properties();
// this sets the property for jetty starting SolrDispatchFilter
if (System.getProperty("solr.data.dir") == null) {
nodeProperties.setProperty("solr.data.dir", createTempDir().toRealPath().toString());
}

solrTestRule.startSolr(homeDir, nodeProperties, JettyConfig.builder().build());
solrTestRule.startSolr(homeDir, new Properties(), JettyConfig.builder().build());
}

@Test
Expand Down
1 change: 0 additions & 1 deletion solr/core/src/test/org/apache/solr/core/TestLazyCores.java
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ public List<CoreDescriptor> discover(CoreContainer cc) {
private CoreContainer init() throws Exception {
solrHomeDirectory = createTempDir();

copyXmlToHome(solrHomeDirectory, "solr.xml");
for (int idx = 1; idx < 10; ++idx) {
copyMinConf(solrHomeDirectory.resolve("collection" + idx));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.util.ArrayList;
import java.util.Properties;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -62,14 +61,9 @@ public final class ReplicationTestHelper {
+ FileSystems.getDefault().getSeparator();

public static JettySolrRunner createAndStartJetty(SolrInstance instance) throws Exception {
Files.copy(
SolrTestCaseJ4.TEST_HOME().resolve("solr.xml"),
Path.of(instance.getHomeDir(), "solr.xml"),
StandardCopyOption.REPLACE_EXISTING);
Properties nodeProperties = new Properties();
nodeProperties.setProperty("solr.data.dir", instance.getDataDir());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess generally you have identified that setting solr.data.dir isn't necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah... I ran the test before and after the cahnge and everythign was green. Then I started looking for other tests that had solr.data.dir and did the same thing...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I at least checked where SolrInstance is used, data dir is the default so, indeed, at least where SolrInstance is used (like here), we don't need to relay it to "solr.data.dir".

JettyConfig jettyConfig = JettyConfig.builder().setPort(0).build();
JettySolrRunner jetty = new JettySolrRunner(instance.getHomeDir(), nodeProperties, jettyConfig);
JettySolrRunner jetty =
new JettySolrRunner(instance.getHomeDir(), new Properties(), jettyConfig);
jetty.start();
return jetty;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,9 @@ public class TestReplicationHandlerBackup extends SolrTestCaseJ4 {

private static JettySolrRunner createAndStartJetty(ReplicationTestHelper.SolrInstance instance)
throws Exception {
Files.copy(
SolrTestCaseJ4.TEST_HOME().resolve("solr.xml"), Path.of(instance.getHomeDir(), "solr.xml"));
Properties nodeProperties = new Properties();
nodeProperties.setProperty("solr.data.dir", instance.getDataDir());
JettyConfig jettyConfig = JettyConfig.builder().setPort(0).build();
JettySolrRunner jetty = new JettySolrRunner(instance.getHomeDir(), nodeProperties, jettyConfig);
JettySolrRunner jetty =
new JettySolrRunner(instance.getHomeDir(), new Properties(), jettyConfig);
jetty.start();
return jetty;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ public void setUp() throws Exception {
originalTestWait = IndexFetcher.testWait;

super.setUp();
// Disable URL allow-list checks for replication tests
System.setProperty("solr.security.allow.urls.enabled", "false");
System.setProperty("solr.directoryFactory", "solr.StandardDirectoryFactory");
String factory =
random().nextInt(100) < 75
Expand Down Expand Up @@ -114,6 +116,7 @@ public void tearDown() throws Exception {
followerClient = null;
}
System.clearProperty(TEST_URL_ALLOW_LIST);
System.clearProperty("solr.security.allow.urls.enabled");
Comment on lines 118 to +119
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearing properties in an @After or @AfterClass is always pointless since our test infra does that for us. Removing them everywhere would be a nice separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, will do that.


IndexFetcher.usableDiskSpaceProvider = originalDiskSpaceprovider;
IndexFetcher.testWait = originalTestWait;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,9 @@ public class TestRestoreCore extends SolrTestCaseJ4 {

private static JettySolrRunner createAndStartJetty(ReplicationTestHelper.SolrInstance instance)
throws Exception {
Files.copy(
SolrTestCaseJ4.TEST_HOME().resolve("solr.xml"), Path.of(instance.getHomeDir(), "solr.xml"));
Properties nodeProperties = new Properties();
nodeProperties.setProperty("solr.data.dir", instance.getDataDir());
JettyConfig jettyConfig = JettyConfig.builder().setPort(0).build();
JettySolrRunner jetty = new JettySolrRunner(instance.getHomeDir(), nodeProperties, jettyConfig);
JettySolrRunner jetty =
new JettySolrRunner(instance.getHomeDir(), new Properties(), jettyConfig);
jetty.start();
return jetty;
}
Expand Down Expand Up @@ -179,7 +176,7 @@ public void testSimpleRestore() throws Exception {
}
}

public void testBackupFailsMissingAllowPaths() throws Exception {
public void testBackupFailsMissingAllowPaths() {
final String params =
"&location=" + URLEncoder.encode(createTempDir().toString(), StandardCharsets.UTF_8);
Throwable t =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

package org.apache.solr.handler;

import java.nio.file.Files;
import java.nio.file.Path;
import org.apache.commons.io.file.PathUtils;
import org.apache.solr.SolrTestCaseJ4;
Expand All @@ -35,7 +34,6 @@ public void testWelcomeMessage() throws Exception {
Path solrHomeTmp = createTempDir();
PathUtils.copyDirectory(
TEST_HOME().resolve("configsets/minimal/conf"), solrHomeTmp.resolve("conf"));
Files.copy(TEST_HOME().resolve("solr.xml"), solrHomeTmp.resolve("solr.xml"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why is solr.xml copying no longer necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno. The test still runs. Sometimes I think that some of this code just got copy/pasted over and over from test to test. Maybe it mattered for one test, and then it just kept living like a zombie. If you are really itnerested I can investigate (i.e ask Little Buddy what the deal is!).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a bit irresponsible to do this on the grounds that tests pass, without looking into the matter any further. Take a look at solr/core/src/test-files/solr/solr.xml. There are comments there about why the settings are different (e.g. distribUpdateSoTimeout) and various customizability that (unfortunately) our standard solr.xml doesn't have (but should).

I can see that the current situation could use improvement. Probably a separate PR should tackle that. Maybe that solr.xml should be in the solr-test-framework JAR as a resource that the test infra can reference and be used by 3rd parties. But also need a simple one-liner to install it into provided Path dir. Or alternatively a new property to the node props which contains a path to where solr.xml is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that I documented the rule's startSolr as follows, meaning it has good test defaults. But only EmbeddedSolrServerTestRule follows this. I suppose then we have room to improve the Jetty side, after which it will indeed make sense to not copy a general test solr.xml.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsmiley I have added https://issues.apache.org/jira/browse/SOLR-18053 to track your suggestion. I've also linked it to a VERY old JIRA of mine, https://issues.apache.org/jira/browse/SOLR-15788 which seeks to get us away from the kitchen sink approach as well.


JettySolrRunner jetty =
new JettySolrRunner(solrHomeTmp.toString(), JettyConfig.builder().build());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ public static void createThings() throws Exception {
.newCollection("collection2")
.withConfigSet(ExternalPaths.TECHPRODUCTS_CONFIGSET)
.create();
var cc = solrTestRule.getCoreContainer();
cc.waitForLoadingCoresToFinish(30000);

String urlCollection1 = solrTestRule.getBaseUrl() + "/" + "collection1";
String urlCollection2 = solrTestRule.getBaseUrl() + "/" + "collection2";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@
*/
public class JerseyApplicationSharingTest extends SolrCloudTestCase {

private static final String collection = "collection1";
private static final String confDir = collection + "/conf";

@BeforeClass
public static void setupCluster() throws Exception {
configureCluster(1)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
import org.apache.lucene.tests.util.LuceneTestCase;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.client.solrj.SolrClient;
Expand All @@ -51,8 +50,6 @@ public static void beforeClass() throws Exception {
solrTestRule.startSolr(LuceneTestCase.createTempDir());
solrTestRule.newCollection("core1").withConfigSet(ExternalPaths.DEFAULT_CONFIGSET).create();
solrTestRule.newCollection("core2").withConfigSet(ExternalPaths.DEFAULT_CONFIGSET).create();
var cc = solrTestRule.getCoreContainer();
cc.waitForLoadingCoresToFinish(30000);

// Populate request metrics on both cores
ModifiableSolrParams queryParams = new ModifiableSolrParams();
Expand Down Expand Up @@ -88,7 +85,7 @@ public void testPrometheusStructureOutput() throws Exception {
seenTypeInfo.add(line));
return false;
})
.collect(Collectors.toList());
.toList();
filteredResponse.forEach(
(actualMetric) -> {
String actualValue;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import java.util.Map;
import java.util.Properties;
import java.util.regex.Pattern;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.embedded.EmbeddedSolrServer;
import org.apache.solr.client.solrj.impl.CloudSolrClient;
Expand Down Expand Up @@ -80,7 +79,6 @@ private static void initStandalone() throws Exception {
final Path collDir = homeDir.resolve("collection1");
final Path confDir = collDir.resolve("conf");
Files.createDirectories(confDir);
Files.copy(SolrTestCaseJ4.TEST_HOME().resolve("solr.xml"), homeDir.resolve("solr.xml"));
String src_dir = TEST_HOME() + "/collection1/conf";
Files.copy(Path.of(src_dir, "schema_latest.xml"), confDir.resolve("schema.xml"));
Files.copy(Path.of(src_dir, "solrconfig-minimal.xml"), confDir.resolve("solrconfig.xml"));
Expand All @@ -96,11 +94,9 @@ private static void initStandalone() throws Exception {
Files.copy(Path.of(src_dir, file), confDir.resolve(file));
}
Files.createFile(collDir.resolve("core.properties"));
Properties nodeProperties = new Properties();
nodeProperties.setProperty("solr.data.dir", h.getCore().getDataDir());
JSR =
new JettySolrRunner(
homeDir.toAbsolutePath().toString(), nodeProperties, JettyConfig.builder().build());
homeDir.toAbsolutePath().toString(), new Properties(), JettyConfig.builder().build());
}

private static void initCloud() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public void testSimple() throws Exception {
assertEquals(3, res.size());
assertEquals(3, beans.size());
for (SolrDocument d : res) {
Integer id = Integer.parseInt(d.getFieldValue("id").toString());
int id = Integer.parseInt(d.getFieldValue("id").toString());
for (String field : new String[] {"data", "data_dv"}) {
byte[] data = (byte[]) d.getFieldValue(field);
if (id == 1) {
Expand All @@ -116,7 +116,7 @@ public void testSimple() throws Exception {
}
}
for (Bean d : beans) {
Integer id = Integer.parseInt(d.id);
int id = Integer.parseInt(d.id);
for (byte[] data : new byte[][] {d.data, d.data_dv}) {
if (id == 1) {
assertEquals(5, data.length);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@

import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Base64;
import java.util.Collections;
Expand Down Expand Up @@ -53,9 +52,6 @@
public class BasicAuthStandaloneTest extends SolrTestCaseJ4 {

private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
private static final Path ROOT_DIR = TEST_HOME();
private static final Path CONF_DIR =
ROOT_DIR.resolve("configsets").resolve("configset-2").resolve("conf");
Comment on lines -57 to -58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears you've changed this test to no longer use configset-2. Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I removed the vairuable and it still ran... I went and looked and yeah, they pretty much are the same:

Solrconfig files: Nearly identical except for a comment difference:

configset-1: Generic comment about being a "kitchen sink" config file for tests
configset-2: Specific comment stating "Small solrconfig with no /get handler defined, for use in TestConfigSets#testConfigSetOnReload"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets not change this in this PR.
"collection1" is the default, not "configset-1". It is an unfortunate kitchen-sink, with many tests using it. I'd rather see tests use one of several basic / standard configs without any quirks and then if needed modify it at runtime using config edit API or another approach on the fly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've backed out the change to this class... I think moving this class to using the kitchen sink collection1 is better than just randomly using configset-2, which was meant for only the TestConfigSets class, but I'd rather get this PR in! I do agree we need to deal with the kitchen-sink issue!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into the history of configset2 when I wrote my comment. I see it as a general addition for something simple. It has a bad name! Other tests now use it too. I didn't see it as a configset that was designed fundamentally and only for TestConfigSets. Maybe I overlooked something... why do you think it's only suitable for that test?


SecurityConfHandlerLocalForTesting securityConfHandler;
SolrInstance instance = null;
Expand Down Expand Up @@ -198,11 +194,9 @@ public static void setBasicAuthHeader(AbstractHttpMessage httpMsg, String user,
}

static JettySolrRunner createAndStartJetty(SolrInstance instance) throws Exception {
Properties nodeProperties = new Properties();
nodeProperties.setProperty("solr.data.dir", instance.getDataDir().toString());
JettySolrRunner jetty =
new JettySolrRunner(
instance.getHomeDir().toString(), nodeProperties, JettyConfig.builder().build());
instance.getHomeDir().toString(), new Properties(), JettyConfig.builder().build());
jetty.start();
return jetty;
}
Expand All @@ -211,7 +205,6 @@ static class SolrInstance {
String name;
Integer port;
Path homeDir;
Path confDir;
Path dataDir;

/**
Expand All @@ -227,36 +220,13 @@ public Path getHomeDir() {
return homeDir;
}

public Path getSchemaFile() {
return CONF_DIR.resolve("schema.xml");
}

public Path getDataDir() {
return dataDir;
}

public Path getSolrConfigFile() {
return CONF_DIR.resolve("solrconfig.xml");
}

public Path getSolrXmlFile() {
return ROOT_DIR.resolve("solr.xml");
}

public void setUp() throws Exception {
homeDir = createTempDir(name).toAbsolutePath();
dataDir = homeDir.resolve("collection1").resolve("data");
confDir = homeDir.resolve("collection1").resolve("conf");

Files.createDirectories(homeDir);
Files.createDirectories(dataDir);
Files.createDirectories(confDir);

Files.copy(getSolrXmlFile(), homeDir.resolve("solr.xml"));
Files.copy(getSolrConfigFile(), confDir.resolve("solrconfig.xml"));
Files.copy(getSchemaFile(), confDir.resolve("schema.xml"));

Files.createFile(homeDir.resolve("collection1").resolve("core.properties"));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -278,10 +278,6 @@ public String getHomeDir() {
return homeDir.toString();
}

public String getUrl() {
return buildUrl(port) + "/collection1";
}

public String getBaseUrl() {
return buildUrl(port);
}
Expand All @@ -294,10 +290,6 @@ public String getSchemaFile() {
return "solrj/solr/collection1/conf/schema-replication1.xml";
}

public String getConfDir() {
return confDir.toString();
}

public String getDataDir() {
return dataDir.toString();
}
Expand All @@ -306,17 +298,11 @@ public String getSolrConfigFile() {
return "solrj/solr/collection1/conf/solrconfig-follower1.xml";
}

public String getSolrXmlFile() {
return "solrj/solr/solr.xml";
}

public void setUp() throws Exception {
Files.createDirectories(homeDir);
Files.createDirectories(dataDir);
Files.createDirectories(confDir);

Files.copy(SolrTestCaseJ4.getFile(getSolrXmlFile()), homeDir.resolve("solr.xml"));

Path f = confDir.resolve("solrconfig.xml");
Files.copy(SolrTestCaseJ4.getFile(getSolrConfigFile()), f);
f = confDir.resolve("schema.xml");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,10 +256,6 @@ public String getHomeDir() {
return homeDir.toString();
}

public String getUrl() {
return buildUrl(port) + "/collection1";
}

public String getBaseUrl() {
return buildUrl(port);
}
Expand All @@ -272,10 +268,6 @@ public String getSchemaFile() {
return "solrj/solr/collection1/conf/schema-replication1.xml";
}

public String getConfDir() {
return confDir.toString();
}

public String getDataDir() {
return dataDir.toString();
}
Expand All @@ -284,17 +276,11 @@ public String getSolrConfigFile() {
return "solrj/solr/collection1/conf/solrconfig-follower1.xml";
}

public String getSolrXmlFile() {
return "solrj/solr/solr.xml";
}

public void setUp() throws Exception {
Files.createDirectories(homeDir);
Files.createDirectories(dataDir);
Files.createDirectories(confDir);

Files.copy(SolrTestCaseJ4.getFile(getSolrXmlFile()), homeDir.resolve("solr.xml"));

Path f = confDir.resolve("solrconfig.xml");
Files.copy(SolrTestCaseJ4.getFile(getSolrConfigFile()), f);
f = confDir.resolve("schema.xml");
Expand Down
Loading