dsmiley commented on code in PR #2924:
URL: https://github.com/apache/solr/pull/2924#discussion_r1936591016
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -410,7 +410,9 @@ private void upgradeToManagedSchema() {
"nor under SolrConfig.getConfigDir() or the current directory. ",
"PLEASE REMOVE THIS FILE.");
} else {
- Path upgradedSchemaFile = Path.of(nonManagedSchemaFile +
UPGRADED_SCHEMA_EXTENSION);
+ Path upgradedSchemaFile =
+ nonManagedSchemaFile.resolveSibling(
Review Comment:
I love the use of resolveSibling where you've introduced it!
##########
solr/core/src/java/org/apache/solr/core/DirectoryFactory.java:
##########
@@ -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);
+ 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());
Review Comment:
I just noticed this reverseOrder; you didn't retain this detail.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -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);
+
+ // 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
Review Comment:
If you want to move it, I think it's okay to do it in this PR as it seems
like a minor detail. But I'm confused at the current state; where is the
calculation occurring? I see hard-coded 0 now.
##########
solr/core/src/java/org/apache/solr/core/SolrPaths.java:
##########
@@ -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("\\\\")) {
Review Comment:
but OR means that this would consistently fail on Windows *even if the path
isn't UNC*
##########
solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java:
##########
@@ -372,12 +372,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);
+ 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(Path.of("conf").resolve(resource).toString());
+ is =
+ classLoader.getResourceAsStream(
+ ("conf/" +
resource).replace(FileSystems.getDefault().getSeparator(), "/"));
Review Comment:
the replacement can be better targeted on resource. It will have no effect
on the `conf/` part.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -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);
Review Comment:
really minor but I think dataDirPath is a better name
##########
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##########
@@ -245,14 +247,14 @@ boolean fetchFromAnyNode() {
}
public Path realPath() {
- return getRealpath(path);
+ return getRealPath(path);
}
@SuppressWarnings("unchecked")
MetaData readMetaData() throws IOException {
- File file = getRealpath(getMetaPath()).toFile();
- if (file.exists()) {
- try (InputStream fis = new FileInputStream(file)) {
+ Path file = getRealPath(getMetaPath());
+ if (Files.exists(file)) {
+ try (InputStream fis = new FileInputStream(file.toString())) {
Review Comment:
Lets just use Files.newInputStream here
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminOperation.java:
##########
@@ -357,7 +356,7 @@ public static NamedList<Object> getCoreStatus(
if (core != null) {
info.add(NAME, core.getName());
info.add("instanceDir", core.getInstancePath().toString());
- info.add("dataDir", normalizePath(core.getDataDir()));
+ info.add("dataDir", Path.of(core.getDataDir()).toString());
Review Comment:
can you bring the method back please? In a file utility place, and
documenting what sort of normalization it does (otherwise is ambiguous). Like;
normalizeToOsPathSeparator(...)
##########
solr/core/src/java/org/apache/solr/handler/admin/ShowFileRequestHandler.java:
##########
@@ -195,54 +195,69 @@ private void showFromZooKeeper(
}
// Return the file indicated (or the directory listing) from the local file
system.
- private void showFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp)
{
+ private void showFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp)
throws IOException {
Path admin = getAdminFileFromFileSystem(req, rsp, hiddenFiles);
if (admin == null) { // exception already recorded
return;
}
- // TODO SOLR-8282 move to PATH
- File adminFile = admin.toFile();
+ Path adminFile = admin;
// Make sure the file exists, is readable and is not a hidden file
- if (!adminFile.exists()) {
- log.error("Can not find: {} [{}]", adminFile.getName(),
adminFile.getAbsolutePath());
+ if (!Files.exists(adminFile)) {
+ log.error("Can not find: {} [{}]", adminFile.getFileName(),
adminFile.toAbsolutePath());
rsp.setException(
new SolrException(
ErrorCode.NOT_FOUND,
- "Can not find: " + adminFile.getName() + " [" +
adminFile.getAbsolutePath() + "]"));
+ "Can not find: "
+ + adminFile.getFileName()
+ + " ["
+ + adminFile.toAbsolutePath()
+ + "]"));
return;
}
- if (!adminFile.canRead() || adminFile.isHidden()) {
- log.error("Can not show: {} [{}]", adminFile.getName(),
adminFile.getAbsolutePath());
+ if (!Files.isReadable(adminFile) || Files.isHidden(adminFile)) {
+ log.error("Can not show: {} [{}]", adminFile.getFileName(),
adminFile.toAbsolutePath());
rsp.setException(
new SolrException(
ErrorCode.NOT_FOUND,
- "Can not show: " + adminFile.getName() + " [" +
adminFile.getAbsolutePath() + "]"));
+ "Can not show: "
+ + adminFile.getFileName()
+ + " ["
+ + adminFile.toAbsolutePath()
+ + "]"));
return;
}
// Show a directory listing
- if (adminFile.isDirectory()) {
+ if (Files.isDirectory(adminFile)) {
// it's really a directory, just go for it.
- int basePath = adminFile.getAbsolutePath().length() + 1;
NamedList<SimpleOrderedMap<Object>> files = new SimpleOrderedMap<>();
- for (File f : adminFile.listFiles()) {
- String path = f.getAbsolutePath().substring(basePath);
- path = path.replace('\\', '/'); // normalize slashes
Review Comment:
Okay, I agree that the ShowFileRequestHandler ought not to be receiving
backslashes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
