dsmiley commented on code in PR #2924:
URL: https://github.com/apache/solr/pull/2924#discussion_r1910436766
##########
solr/core/src/java/org/apache/solr/core/CoreDescriptor.java:
##########
@@ -96,7 +95,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(),
Review Comment:
The trailing slash should ideally not be necessary. I'd prefer to just drop
it and we deal with the consequences. **If** needed, the previous code was
clearer IMO so I'd rather see something similarly clear.
##########
solr/core/src/java/org/apache/solr/cloud/ZkSolrResourceLoader.java:
##########
@@ -121,7 +120,7 @@ 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(Path.of(resource).toString());
Review Comment:
Assuming that actually works, I think a little comment is needed like
"normalize path separator"
##########
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:
I'm suspicious of this technique. I'd prefer we have a utility method to
make certain normalizations when they are needed.
##########
solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java:
##########
@@ -404,8 +406,7 @@ public String resourceLocation(String resource) {
return inInstanceDir.normalize().toString();
}
- try (InputStream is =
- classLoader.getResourceAsStream(resource.replace(File.separatorChar,
'/'))) {
+ try (InputStream is =
classLoader.getResourceAsStream(Path.of(resource).toString())) {
Review Comment:
On Windows, wouldn't this actually *convert* to the platform's separator
char?
##########
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
-
- if (isHiddenFile(req, rsp, f.getName().replace('\\', '/'), false,
hiddenFiles)) {
- continue;
- }
-
- SimpleOrderedMap<Object> fileInfo = new SimpleOrderedMap<>();
- files.add(path, fileInfo);
- if (f.isDirectory()) {
- fileInfo.add("directory", true);
- } else {
- // TODO? content type
- fileInfo.add("size", f.length());
- }
- fileInfo.add("modified", new Date(f.lastModified()));
+ try (Stream<Path> directoryFiles = Files.list(adminFile)) {
+ directoryFiles.forEach(
Review Comment:
no try-with-resources needed
##########
solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java:
##########
@@ -353,12 +353,14 @@ 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.");
+ String pathResource = Paths.get(resource).normalize().toString();
+ if (pathResource.trim().startsWith("\\\\")) { // Always disallow UNC paths
Review Comment:
why trim after we've called normalize? If trimming had been first thing;
probably needs to continue to be first thing. I don't like trim in most places
including here though (it's generally sloppy); in a major version we can get
away with dropping it.
+1 to Eric's comment. And no need to call normalize at this spot.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1124,6 +1121,9 @@ protected SolrCore(
this.snapshotMgr = initSnapshotMetaDataManager();
this.solrDelPolicy = initDeletionPolicy(delPolicy);
+ // initialize core metrics
+ initializeMetrics(solrMetricsContext, null);
Review Comment:
why did you move this?
##########
solr/core/src/java/org/apache/solr/core/CoreDescriptor.java:
##########
@@ -64,7 +63,7 @@ public class CoreDescriptor {
public static final String SOLR_CORE_PROP_PREFIX = "solr.core.";
public static final String DEFAULT_EXTERNAL_PROPERTIES_FILE =
Review Comment:
Ideally we change Strings to Paths like this. If too hard, can defer.
##########
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##########
@@ -1484,28 +1484,27 @@ private void copyTmpConfFiles2Conf(Path tmpconfDir) {
int numTempPathElements = tmpconfDir.getNameCount();
for (Path path : makeTmpConfDirFileList(tmpconfDir)) {
Path oldPath = confPath.resolve(path.subpath(numTempPathElements,
path.getNameCount()));
+
try {
Files.createDirectories(oldPath.getParent());
} catch (IOException e) {
throw new SolrException(
ErrorCode.SERVER_ERROR, "Unable to mkdirs: " +
oldPath.getParent(), e);
}
if (Files.exists(oldPath)) {
- File oldFile = oldPath.toFile(); // TODO SOLR-8282 move to PATH
- File backupFile =
- new File(oldFile.getPath() + "." + getDateAsStr(new
Date(oldFile.lastModified())));
- if (!backupFile.getParentFile().exists()) {
- status = backupFile.getParentFile().mkdirs();
- if (!status) {
- throw new SolrException(
- ErrorCode.SERVER_ERROR, "Unable to mkdirs: " +
backupFile.getParentFile());
+ try {
+ Path backupFile =
+ Path.of(
+ oldPath
+ + "."
+ + getDateAsStr(new
Date(Files.getLastModifiedTime(oldPath).toMillis())));
+ if (!Files.exists(backupFile.getParent())) {
+ Files.createDirectories(backupFile.getParent());
}
Review Comment:
No need to check if doesn't exist; an advantage of Files.createDirectories
##########
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());
+ }
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(
Review Comment:
well done
##########
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##########
@@ -85,14 +85,14 @@ public Path getRealpath(String path) {
}
private static Path _getRealPath(String path, Path solrHome) {
- if (File.separatorChar == '\\') {
- path = path.replace('/', File.separatorChar);
- }
- SolrPaths.assertNotUnc(Path.of(path));
- while (path.startsWith(File.separator)) { // Trim all leading slashes
+ Path dir = Path.of(path);
+ SolrPaths.assertNotUnc(dir);
+
+ while (path.startsWith("/")) {
path = path.substring(1);
}
Review Comment:
This beginning of this method is awkward. Ultimately we want to resolve
path against SolrHome, while insisting that path is relative, even if an
absolute path was passed in. (gosh I wish a comment was there saying this). I
kinda wish we could declare that as a requirement to callers so they didn't
pass absolute paths (up to you; maybe scope creep). So we strip off any
leading path separator chars, thus converting it to a relative path. If we do
that, UNC will be impossible.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1354,16 +1354,29 @@ 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");
+ Path dataDirFile = Paths.get(dataDir);
+ long totalSpace;
+ long useableSpace;
+
+ try {
+ totalSpace = Files.getFileStore(dataDirFile).getTotalSpace();
+ useableSpace = Files.getFileStore(dataDirFile).getUsableSpace();
+ } catch (Exception e) {
+ // TODO Metrics used to default to 0 with java.io.File even if directory
didn't exist
+ // Should throw an exception and initialize data directory before metrics
+ totalSpace = 0L;
+ useableSpace = 0L;
+ }
Review Comment:
Do not pre-compute this stuff; it should only be calculated/fetched when the
metric is fetched. Maybe could clarify that the TODO is to move metrics
initialization to after data directory initialization, thus avoiding this
exception (hopefully) but that may not be entirely possible.
##########
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:
don't lose this. I feel like we need a toForwardSlashPathString(Path)
utility method.
##########
solr/core/src/java/org/apache/solr/core/SolrPaths.java:
##########
@@ -43,9 +42,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;
+ return (path != null && (!(path.endsWith("/") || path.endsWith("\\")))) ?
path + "/" : path;
Review Comment:
The code was fine as it was but perhaps got your attention because File is
referenced. Since we only need to know the separator, try
`FileSystems.getDefault().getSeparator()`
##########
solr/core/src/java/org/apache/solr/core/backup/BackupManager.java:
##########
@@ -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());
Review Comment:
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.
##########
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##########
@@ -428,10 +428,10 @@ public boolean fetch(String path, String from) {
@Override
public void get(String path, Consumer<FileEntry> consumer, boolean
fetchmissing)
throws IOException {
- File file = getRealpath(path).toFile();
- String simpleName = file.getName();
+ Path file = getRealpath(path);
+ String simpleName = file.getFileName().toString();
if (isMetaDataFile(simpleName)) {
- try (InputStream is = new FileInputStream(file)) {
+ try (InputStream is = new FileInputStream(file.toString())) {
Review Comment:
Yes Eric is right but may want to do that universally in another PR to avoid
scope creep.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
