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.

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
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@
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;
import java.nio.file.FileSystems;
import java.nio.file.Path;
import org.apache.solr.common.SolrException.ErrorCode;
import org.apache.solr.common.cloud.ZooKeeperException;
Expand Down Expand Up @@ -121,7 +121,9 @@ 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, '/'));
is =
classLoader.getResourceAsStream(
resource.replace(FileSystems.getDefault().getSeparator(), "/"));
} catch (Exception e) {
throw new IOException("Error opening " + resource, e);
}
Expand Down
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
6 changes: 3 additions & 3 deletions solr/core/src/java/org/apache/solr/core/CoreDescriptor.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
*/
package org.apache.solr.core;

import java.io.File;
import java.io.IOException;
import java.io.Reader;
import java.lang.invoke.MethodHandles;
import java.nio.charset.StandardCharsets;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.HashMap;
Expand Down Expand Up @@ -64,7 +64,7 @@ public class CoreDescriptor {
public static final String SOLR_CORE_PROP_PREFIX = "solr.core.";

public static final String DEFAULT_EXTERNAL_PROPERTIES_FILE =
dsmiley marked this conversation as resolved.
Show resolved Hide resolved
"conf" + File.separator + "solrcore.properties";
"conf" + FileSystems.getDefault().getSeparator() + "solrcore.properties";

/**
* Whether this core was configured using a configSet that was trusted. This helps in avoiding the
Expand Down Expand Up @@ -96,7 +96,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, "data" + FileSystems.getDefault().getSeparator(),
CORE_TRANSIENT, "false",
CORE_LOADONSTARTUP, "true");

Expand Down
93 changes: 50 additions & 43 deletions solr/core/src/java/org/apache/solr/core/DirectoryFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,15 @@
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.Comparator;
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 +336,68 @@ 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(Comparator.reverseOrder())
.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]);
if (log.isInfoEnabled())
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,
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);
}
}

if (log.isInfoEnabled())
mlbiscoc marked this conversation as resolved.
Show resolved Hide resolved
log.info(
"Found {} old index directories to clean-up under {} afterReload={}",
dirsList.size() - i,
dataDirPath,
afterCoreReload);

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
31 changes: 24 additions & 7 deletions solr/core/src/java/org/apache/solr/core/SolrCore.java
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 All @@ -33,9 +32,9 @@
import java.lang.invoke.MethodHandles;
import java.lang.reflect.Constructor;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -1354,14 +1353,32 @@ 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();
Path dataDirPath = Path.of(dataDir);
parentContext.gauge(
() -> dataDirFile.getTotalSpace(), true, "totalSpace", Category.CORE.toString(), "fs");
() -> {
try {
return Files.getFileStore(dataDirPath).getTotalSpace();
} catch (IOException e) {
return 0L;
}
},
true,
"totalSpace",
Category.CORE.toString(),
"fs");
parentContext.gauge(
() -> dataDirFile.getUsableSpace(), true, "usableSpace", Category.CORE.toString(), "fs");
() -> {
try {
return Files.getFileStore(dataDirPath).getUsableSpace();
} catch (IOException e) {
return 0L;
}
},
true,
"usableSpace",
Category.CORE.toString(),
"fs");
parentContext.gauge(
() -> dataDirPath.toAbsolutePath().toString(),
true,
Expand Down
4 changes: 2 additions & 2 deletions solr/core/src/java/org/apache/solr/core/SolrPaths.java
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
13 changes: 9 additions & 4 deletions solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java
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 @@ -373,12 +373,16 @@ 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.replace(FileSystems.getDefault().getSeparator(), "/"));

// 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(
("conf/" + resource.replace(FileSystems.getDefault().getSeparator(), "/")));
}

if (is == null) {
Expand All @@ -405,7 +409,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
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.FileSystems;
import java.time.Instant;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -343,7 +343,8 @@ 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, filePath.replace("/", FileSystems.getDefault().getSeparator()));
// checking for '/' is correct for a directory since ConfigSetService#getAllConfigFiles
// always separates file paths with '/'
if (!filePath.endsWith("/")) {
Expand Down
Loading