Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOLR-16903: Migrate off java.io.File to java.nio.file.Path from core files #2924

Merged
merged 10 commits into from
Feb 1, 2025
Merged

SOLR-16903: Migrate off java.io.File to java.nio.file.Path from core files #2924

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6cc27d7
Inital core File to Path commit
mlbiscoc Dec 22, 2024
bc2db00
Move initialize metrics until after data dir was created
mlbiscoc Dec 23, 2024
4c01109
Compile failure
mlbiscoc Dec 23, 2024
f17b809
Fix to bad conversions
mlbiscoc Dec 23, 2024
36d0b92
Sort list in getLatestFile
mlbiscoc Dec 23, 2024
374a4a1
Refactor
mlbiscoc Dec 24, 2024
63fb99d
First round addressing PR comments
mlbiscoc Jan 16, 2025
fb4daf3
Second round of PR comments
mlbiscoc Jan 27, 2025
5f8abae
Small refactoring and fix with metrics
Jan 31, 2025
679fb29
Update CHANGES.txt
Jan 31, 2025
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
4 changes: 2 additions & 2 deletions solr/core/src/java/org/apache/solr/cloud/ZkSolrResourceLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package org.apache.solr.cloud;

import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.lang.invoke.MethodHandles;
Expand Down Expand Up @@ -121,7 +120,8 @@ public InputStream openResource(String resource) throws IOException {

try {
// delegate to the class loader (looking into $INSTANCE_DIR/lib jars)
is = classLoader.getResourceAsStream(resource.replace(File.separatorChar, '/'));
// Use Path to normalize path separator
is = classLoader.getResourceAsStream(Path.of(resource).toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming that actually works, I think a little comment is needed like "normalize path separator"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

Copy link
Contributor

Choose a reason for hiding this comment

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

But I think it doesn't work; not on Windows. I thought I commented elsewhere in that lengthy review that all Path.of(resource).toString() that you are adding is probably flawed

} catch (Exception e) {
throw new IOException("Error opening " + resource, e);
}
Expand Down
6 changes: 1 addition & 5 deletions solr/core/src/java/org/apache/solr/core/CachingDirectoryFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
*/
package org.apache.solr.core;

import java.io.File;
import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.nio.file.DirectoryStream;
Expand Down Expand Up @@ -367,10 +366,7 @@ private void close(CacheValue val) {
}

private static boolean isSubPath(CacheValue cacheValue, CacheValue otherCacheValue) {
int one = cacheValue.path.lastIndexOf(File.separatorChar);
int two = otherCacheValue.path.lastIndexOf(File.separatorChar);

return otherCacheValue.path.startsWith(cacheValue.path + File.separatorChar) && two > one;
return Path.of(otherCacheValue.path).startsWith(Path.of(cacheValue.path));
mlbiscoc marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
Expand Down
9 changes: 4 additions & 5 deletions solr/core/src/java/org/apache/solr/core/CoreDescriptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
*/
package org.apache.solr.core;

import java.io.File;
import java.io.IOException;
import java.io.Reader;
import java.lang.invoke.MethodHandles;
Expand Down Expand Up @@ -63,8 +62,7 @@ public class CoreDescriptor {
public static final String CORE_CONFIGSET_PROPERTIES = "configSetProperties";
public static final String SOLR_CORE_PROP_PREFIX = "solr.core.";

public static final String DEFAULT_EXTERNAL_PROPERTIES_FILE =
"conf" + File.separator + "solrcore.properties";
public static final Path DEFAULT_EXTERNAL_PROPERTIES_FILE = Path.of("conf/solrcore.properties");

/**
* Whether this core was configured using a configSet that was trusted. This helps in avoiding the
Expand Down Expand Up @@ -96,7 +94,7 @@ public Properties getPersistableUserProperties() {
CORE_CONFIG, "solrconfig.xml",
CORE_SCHEMA, "schema.xml",
CORE_CONFIGSET_PROPERTIES, ConfigSetProperties.DEFAULT_FILENAME,
CORE_DATADIR, "data" + File.separator,
CORE_DATADIR, Path.of("data").toString(),
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 the Path conversion is pointless here. Just use the separator char in the default FileSystem impl. I don't love the trailing slash but changing it is perhaps not the right PR for that.

CORE_TRANSIENT, "false",
CORE_LOADONSTARTUP, "true");

Expand Down Expand Up @@ -240,7 +238,8 @@ public CoreDescriptor(
* <p>File paths are taken as read from the core's instance directory if they are not absolute.
*/
protected void loadExtraProperties() {
String filename = coreProperties.getProperty(CORE_PROPERTIES, DEFAULT_EXTERNAL_PROPERTIES_FILE);
String filename =
coreProperties.getProperty(CORE_PROPERTIES, DEFAULT_EXTERNAL_PROPERTIES_FILE.toString());
Path propertiesFile = instanceDir.resolve(filename);
if (Files.exists(propertiesFile)) {
try (Reader r = Files.newBufferedReader(propertiesFile, StandardCharsets.UTF_8)) {
Expand Down
82 changes: 43 additions & 39 deletions solr/core/src/java/org/apache/solr/core/DirectoryFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,14 @@
package org.apache.solr.core;

import java.io.Closeable;
import java.io.File;
import java.io.FileFilter;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.nio.file.Files;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.stream.Stream;
import org.apache.commons.io.file.PathUtils;
import org.apache.lucene.store.Directory;
import org.apache.lucene.store.FilterDirectory;
Expand Down Expand Up @@ -338,59 +335,66 @@ public String getDataHome(CoreDescriptor cd) throws IOException {
}

public void cleanupOldIndexDirectories(
final String dataDirPath, final String currentIndexDirPath, boolean afterCoreReload) {
final String dataDirPath, final String currentIndexDirPath, boolean afterCoreReload)
throws IOException {

// TODO SOLR-8282 move to PATH
File dataDir = new File(dataDirPath);
if (!dataDir.isDirectory()) {
Path dataDirFile = Path.of(dataDirPath);
mlbiscoc marked this conversation as resolved.
Show resolved Hide resolved
if (!Files.isDirectory(dataDirFile)) {
log.debug(
"{} does not point to a valid data directory; skipping clean-up of old index directories.",
dataDirPath);
return;
}

final File currentIndexDir = new File(currentIndexDirPath);
File[] oldIndexDirs =
dataDir.listFiles(
new FileFilter() {
@Override
public boolean accept(File file) {
String fileName = file.getName();
return file.isDirectory()
&& !file.equals(currentIndexDir)
&& (fileName.equals("index") || fileName.matches(INDEX_W_TIMESTAMP_REGEX));
}
});
final Path currentIndexDir = Path.of(currentIndexDirPath);
List<Path> dirsList;
try (Stream<Path> oldIndexDirs = Files.list(dataDirFile)) {
dirsList =
oldIndexDirs
.filter(
(file) -> {
String fileName = file.getFileName().toString();
return Files.isDirectory(file)
&& !file.equals(currentIndexDir)
&& (fileName.equals("index") || fileName.matches(INDEX_W_TIMESTAMP_REGEX));
})
.sorted()
.toList();
}
;

if (oldIndexDirs == null || oldIndexDirs.length == 0)
if (dirsList.isEmpty()) {
return; // nothing to do (no log message needed)

List<File> dirsList = Arrays.asList(oldIndexDirs);
dirsList.sort(Collections.reverseOrder());
dsmiley marked this conversation as resolved.
Show resolved Hide resolved
}

int i = 0;
if (afterCoreReload) {
log.info("Will not remove most recent old directory after reload {}", oldIndexDirs[0]);
log.info("Will not remove most recent old directory after reload {}", dirsList.getFirst());
i = 1;
}

log.info(
"Found {} old index directories to clean-up under {} afterReload={}",
oldIndexDirs.length - i,
dirsList.size() - i,
dataDirPath,
afterCoreReload);
for (; i < dirsList.size(); i++) {
File dir = dirsList.get(i);
String dirToRmPath = dir.getAbsolutePath();
try {
if (deleteOldIndexDirectory(dirToRmPath)) {
log.info("Deleted old index directory: {}", dirToRmPath);
} else {
log.warn("Delete old index directory {} failed.", dirToRmPath);
}
} catch (IOException ioExc) {
log.error("Failed to delete old directory {} due to: ", dir.getAbsolutePath(), ioExc);
}
}

dirsList.stream()
.skip(i)
.forEach(
mlbiscoc marked this conversation as resolved.
Show resolved Hide resolved
(entry) -> {
String dirToRmPath = entry.toAbsolutePath().toString();
try {
if (deleteOldIndexDirectory(dirToRmPath)) {
log.info("Deleted old index directory: {}", dirToRmPath);
} else {
log.warn("Delete old index directory {} failed.", dirToRmPath);
}
} catch (IOException ioExc) {
log.error(
"Failed to delete old directory {} due to: ", entry.toAbsolutePath(), ioExc);
}
});
}

// Extension point to allow subclasses to infuse additional code when deleting old index
Expand Down
19 changes: 10 additions & 9 deletions solr/core/src/java/org/apache/solr/core/SolrCore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import com.codahale.metrics.Counter;
import com.codahale.metrics.Timer;
import java.io.Closeable;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
Expand Down Expand Up @@ -1354,16 +1353,18 @@ public void initializeMetrics(SolrMetricsContext parentContext, String scope) {
Category.CORE.toString());
}

// TODO SOLR-8282 move to PATH
// initialize disk total / free metrics
Path dataDirPath = Paths.get(dataDir);
File dataDirFile = dataDirPath.toFile();
parentContext.gauge(
() -> dataDirFile.getTotalSpace(), true, "totalSpace", Category.CORE.toString(), "fs");
parentContext.gauge(
() -> dataDirFile.getUsableSpace(), true, "usableSpace", Category.CORE.toString(), "fs");
Path dataDirFile = Paths.get(dataDir);
Copy link
Contributor

Choose a reason for hiding this comment

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

really minor but I think dataDirPath is a better name


// Do not pre-compute the data directory total/usable space on initialization. Calculated when
// metric is fetched
// TODO Move metrics initialization calls to after the data directory is created to fetch true
dsmiley marked this conversation as resolved.
Show resolved Hide resolved
// directory space on initialization
parentContext.gauge(() -> 0L, true, "totalSpace", Category.CORE.toString(), "fs");
parentContext.gauge(() -> 0L, true, "usableSpace", Category.CORE.toString(), "fs");

parentContext.gauge(
() -> dataDirPath.toAbsolutePath().toString(),
() -> dataDirFile.toAbsolutePath().toString(),
true,
"path",
Category.CORE.toString(),
Expand Down
6 changes: 3 additions & 3 deletions solr/core/src/java/org/apache/solr/core/SolrPaths.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@

package org.apache.solr.core;

import java.io.File;
import java.lang.invoke.MethodHandles;
import java.nio.file.FileSystems;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Collections;
Expand All @@ -44,7 +44,7 @@ private SolrPaths() {} // don't create this
/** Ensures a directory name always ends with a '/'. */
public static String normalizeDir(String path) {
return (path != null && (!(path.endsWith("/") || path.endsWith("\\"))))
? path + File.separator
? path + FileSystems.getDefault().getSeparator()
: path;
}

Expand Down Expand Up @@ -94,7 +94,7 @@ public static void assertNoPathTraversal(Path pathToAssert) {

/** Asserts that a path is not a Windows UNC path */
public static void assertNotUnc(Path pathToAssert) {
if (OS.isFamilyWindows() && pathToAssert.toString().startsWith("\\\\")) {
if (OS.isFamilyWindows() || pathToAssert.toString().startsWith("\\\\")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why the change from logical AND to OR?

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 mentioned it in this comment here
This was done because I changed openResource inside of SolrResourceLoader to use this assert method instead of it checking .startsWith("\\\\"). I am assuming from the name its asserting that it is not a UNC path, not necessarily just because of the OS being used but probably isn't right. I would need to see why it was an AND in the history. Maybe this should be called assertNotUNCorWindowsOS or just make a different assert just checking that .startsWith("\\\\") ...

Copy link
Contributor

Choose a reason for hiding this comment

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

but OR means that this would consistently fail on Windows even if the path isn't UNC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats true but currently the assertion is skipped on non-windows even if the path is a UNC path because of the AND when I thought this function was to only check UNC but maybe that was for a reason.

I'm going to revert this and revert openResource back to its original assertion.

throw new SolrException(
SolrException.ErrorCode.BAD_REQUEST,
"Path " + pathToAssert + " disallowed. UNC paths not supported.");
Expand Down
14 changes: 7 additions & 7 deletions solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import com.google.common.annotations.VisibleForTesting;
import java.io.Closeable;
import java.io.File;
import java.io.FilterInputStream;
import java.io.IOException;
import java.io.InputStream;
Expand All @@ -31,6 +30,7 @@
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.nio.file.DirectoryStream;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.PathMatcher;
Expand Down Expand Up @@ -353,9 +353,8 @@ public ClassLoader getClassLoader() {
*/
@Override
public InputStream openResource(String resource) throws IOException {
if (resource.trim().startsWith("\\\\")) { // Always disallow UNC paths
throw new SolrResourceNotFoundException("Resource '" + resource + "' could not be loaded.");
}
SolrPaths.assertNotUnc(Path.of(resource)); // Always disallow UNC paths

Path instanceDir = getInstancePath().normalize();
Path inInstanceDir = getInstancePath().resolve(resource).normalize();
Path inConfigDir = instanceDir.resolve("conf").resolve(resource).normalize();
Expand All @@ -373,12 +372,12 @@ public InputStream openResource(String resource) throws IOException {

// Delegate to the class loader (looking into $INSTANCE_DIR/lib jars).
// We need a ClassLoader-compatible (forward-slashes) path here!
InputStream is = classLoader.getResourceAsStream(resource.replace(File.separatorChar, '/'));
InputStream is = classLoader.getResourceAsStream(resource);
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change? The comments explain why it was needed


// This is a hack just for tests (it is not done in ZKResourceLoader)!
// TODO can we nuke this?
if (is == null && System.getProperty("jetty.testMode") != null) {
is = classLoader.getResourceAsStream(("conf/" + resource).replace(File.separatorChar, '/'));
is = classLoader.getResourceAsStream(Path.of("conf").resolve(resource).toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

again, the classLoader.getResourceAsStream namespace is not exactly a FileSystem but is a forward-slash based scheme (thus conversion is needed). I think it was fine, albeit maybe reference separator char via default FileSystem

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 relooked at File.separatorChar throughout this PR and moved them to FileSystems.getDefault().getSeparator() where it might make sense instead of moving it to PATH to avoid as little breaking as possible.

}

if (is == null) {
Expand All @@ -405,7 +404,8 @@ public String resourceLocation(String resource) {
}

try (InputStream is =
classLoader.getResourceAsStream(resource.replace(File.separatorChar, '/'))) {
classLoader.getResourceAsStream(
resource.replace(FileSystems.getDefault().getSeparator(), "/"))) {
if (is != null) return "classpath:" + resource;
} catch (IOException e) {
// ignore
Expand Down
4 changes: 2 additions & 2 deletions solr/core/src/java/org/apache/solr/core/backup/BackupManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@
*/
package org.apache.solr.core.backup;

import java.io.File;
import java.io.IOException;
import java.io.OutputStream;
import java.io.OutputStreamWriter;
import java.io.Writer;
import java.lang.invoke.MethodHandles;
import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.time.Instant;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -343,7 +343,7 @@ private void downloadConfigToRepo(ConfigSetService configSetService, String conf
// getAllConfigFiles always separates file paths with '/'
for (String filePath : filePaths) {
// Replace '/' to ensure that propre file is resolved for writing.
URI uri = repository.resolve(dir, filePath.replace('/', File.separatorChar));
URI uri = repository.resolve(dir, Path.of(filePath).toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

could repository.resolve() take a path instead of a string?

Copy link
Contributor

Choose a reason for hiding this comment

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

No Eric; the BackupRepository abstraction is based around URI & String. We could change it (maybe that's your proposal) but that's a big change that deserves its own discussion/issue. I'd rather here see the existing code or something very close to it.

// checking for '/' is correct for a directory since ConfigSetService#getAllConfigFiles
// always separates file paths with '/'
if (!filePath.endsWith("/")) {
Expand Down
Loading
Loading