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-17548: Switch all public Java APIs from File to Path #2907

Merged
merged 12 commits into from
Dec 19, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ public void startMiniCluster(int nodeCount) {
cluster =
new MiniSolrCloudCluster.Builder(nodeCount, miniClusterBaseDir)
.formatZkServer(false)
.addConfig("conf", getFile("src/resources/configs/cloud-minimal/conf").toPath())
.addConfig("conf", getFile("src/resources/configs/cloud-minimal/conf"))
Copy link
Contributor

Choose a reason for hiding this comment

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

love seeing the changed version is shorter and simpler than the original one!

.configure();
} catch (Exception e) {
if (Files.exists(miniClusterBaseDir)) {
Expand Down Expand Up @@ -551,25 +551,25 @@ public static ModifiableSolrParams params(ModifiableSolrParams params, String...
* @param name the name
* @return the file
*/
public static File getFile(String name) {
public static Path getFile(String name) {
final URL url =
MiniClusterState.class.getClassLoader().getResource(name.replace(File.separatorChar, '/'));
if (url != null) {
try {
return new File(url.toURI());
return Path.of(url.toURI());
} catch (Exception e) {
throw new RuntimeException(
"Resource was found on classpath, but cannot be resolved to a "
+ "normal file (maybe it is part of a JAR file): "
+ name);
}
}
File file = new File(name);
if (file.exists()) {
Path file = Path.of(name);
if (Files.exists(file)) {
return file;
} else {
file = new File("../../../", name);
if (file.exists()) {
file = Path.of("../../../", name);
if (Files.exists(file)) {
return file;
}
}
Expand Down
9 changes: 4 additions & 5 deletions solr/core/src/java/org/apache/solr/cloud/SolrZkServer.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
*/
package org.apache.solr.cloud;

import java.io.File;
import java.io.IOException;
import java.io.Reader;
import java.lang.invoke.MethodHandles;
Expand Down Expand Up @@ -51,10 +50,10 @@ public class SolrZkServer {

private Thread zkThread; // the thread running a zookeeper server, only if zkRun is set

private File dataHome; // o.a.zookeeper.**.QuorumPeerConfig needs a File not a Path
private Path dataHome; // o.a.zookeeper.**.QuorumPeerConfig needs a File not a Path
private String confHome;

public SolrZkServer(String zkRun, String zkHost, File dataHome, String confHome, int solrPort) {
public SolrZkServer(String zkRun, String zkHost, Path dataHome, String confHome, int solrPort) {
this.zkRun = zkRun;
this.zkHost = zkHost;
this.dataHome = dataHome;
Expand Down Expand Up @@ -277,8 +276,8 @@ public static boolean hasServers(Properties props) {
return false;
}

public void setDataDir(File dataDir) {
this.dataDir = dataDir;
public void setDataDir(Path dataDir) {
this.dataDir = dataDir.toFile();
Copy link
Contributor

Choose a reason for hiding this comment

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

is this an example of where you felt changing things got out of hand? Otherwise, seems like if dataDir was a Path, then no .toFile()??

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 this one is setting dataDir from an import existing in org.apache.zookeeper.server.quorum.QuorumPeerConfig which is outside of Solr. If you see toFile() it is most likely because it either got out of hand and I put TODO to come back later or it wasn't possible to use Path strictly

}

/**
Expand Down
2 changes: 2 additions & 0 deletions solr/core/src/java/org/apache/solr/core/DirectoryFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,8 @@ public String getDataHome(CoreDescriptor cd) throws IOException {

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

// TODO SOLR-8282 move to PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

Your stepwise logic is probably much better than a "lets change it all in one fell swoop!"

File dataDir = new File(dataDirPath);
if (!dataDir.isDirectory()) {
log.debug(
Expand Down
1 change: 1 addition & 0 deletions solr/core/src/java/org/apache/solr/core/SolrCore.java
Original file line number Diff line number Diff line change
Expand Up @@ -1354,6 +1354,7 @@ 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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@
*/
package org.apache.solr.core;

import java.io.File;
import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.nio.file.Path;
import java.util.List;
import org.apache.lucene.index.IndexCommit;
import org.apache.lucene.index.IndexDeletionPolicy;
Expand Down Expand Up @@ -198,8 +198,8 @@ private String getId(IndexCommit commit) {
// For anything persistent, make something that will
// be the same, regardless of the Directory instance.
if (dir instanceof FSDirectory fsd) {
File fdir = fsd.getDirectory().toFile();
sb.append(fdir.getPath());
Path fdir = fsd.getDirectory();
sb.append(fdir.toString());
} else {
sb.append(dir);
}
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.AtomicMoveNotSupportedException;
Expand Down Expand Up @@ -78,7 +77,7 @@ protected LockFactory createLockFactory(String rawLockType) throws IOException {

@Override
public String normalize(String path) throws IOException {
return super.normalize(new File(path).getCanonicalPath());
return super.normalize(Path.of(path).toAbsolutePath().toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

File.getCanonicalPath is not identical to Path.toAbsolutePath. It requires adding normalize().

}

@Override
Expand Down
6 changes: 3 additions & 3 deletions solr/core/src/java/org/apache/solr/core/ZkContainer.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
import static org.apache.solr.common.cloud.ZkStateReader.HTTPS;
import static org.apache.solr.common.cloud.ZkStateReader.HTTPS_PORT_PROP;

import java.io.File;
import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.List;
import java.util.concurrent.ExecutorService;
Expand Down Expand Up @@ -99,7 +99,7 @@ public void initZooKeeper(final CoreContainer cc, CloudConfig config) {
new SolrZkServer(
stripChroot(zkRun),
stripChroot(config.getZkHost()),
new File(zkDataHome),
Path.of(zkDataHome),
zkConfHome,
config.getSolrHostPort());
zkServer.parseConfig();
Expand Down Expand Up @@ -184,7 +184,7 @@ public SolrMetricsContext getSolrMetricsContext() {
}

private String stripChroot(String zkRun) {
if (zkRun == null || zkRun.trim().length() == 0 || zkRun.lastIndexOf('/') < 0) return zkRun;
if (zkRun == null || zkRun.trim().isEmpty() || zkRun.lastIndexOf('/') < 0) return zkRun;
return zkRun.substring(0, zkRun.lastIndexOf('/'));
}

Expand Down
62 changes: 35 additions & 27 deletions solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ IndexFetchResult fetchLatestIndex(boolean forceReplication, boolean forceCoreRel
Directory indexDir = null;
String indexDirPath;
boolean deleteTmpIdxDir = true;
File tmpTlogDir = null;
Path tmpTlogDir = null;

if (!solrCore.getSolrCoreState().getLastReplicateIndexSuccess()) {
// if the last replication was not a success, we force a full replication
Expand Down Expand Up @@ -797,7 +797,7 @@ private void cleanup(
Directory tmpIndexDir,
Directory indexDir,
boolean deleteTmpIdxDir,
File tmpTlogDir,
Path tmpTlogDir,
boolean successfulInstall)
throws IOException {
try {
Expand Down Expand Up @@ -1065,6 +1065,7 @@ private void downloadConfFiles(
confFilesDownloaded = Collections.synchronizedList(new ArrayList<>());
Path tmpConfPath =
solrCore.getResourceLoader().getConfigPath().resolve("conf." + getDateAsStr(new Date()));
// TODO SOLR-8282 move to PATH
File tmpconfDir = tmpConfPath.toFile();
try {
boolean status = tmpconfDir.mkdirs();
Expand All @@ -1076,7 +1077,8 @@ private void downloadConfFiles(
for (Map<String, Object> file : confFilesToDownload) {
String saveAs = (String) (file.get(ALIAS) == null ? file.get(NAME) : file.get(ALIAS));
localFileFetcher =
new LocalFsFileFetcher(tmpconfDir, file, saveAs, CONF_FILE_SHORT, latestGeneration);
new LocalFsFileFetcher(
tmpconfDir.toPath(), file, saveAs, CONF_FILE_SHORT, latestGeneration);
currentFile = file;
localFileFetcher.fetchFile();
confFilesDownloaded.add(new HashMap<>(file));
Expand All @@ -1086,7 +1088,7 @@ private void downloadConfFiles(
terminateAndWaitFsyncService();
copyTmpConfFiles2Conf(tmpConfPath);
} finally {
delTree(tmpconfDir);
delTree(tmpconfDir.toPath());
}
}

Expand Down Expand Up @@ -1153,6 +1155,7 @@ private long downloadIndexFiles(
alwaysDownload);
}
if (!compareResult.equal || downloadCompleteIndex || alwaysDownload) {
// TODO SOLR-8282 move to PATH
File localFile = new File(indexDirPath, filename);
if (downloadCompleteIndex
&& doDifferentialCopy
Expand Down Expand Up @@ -1200,15 +1203,15 @@ private long downloadIndexFiles(

private static Long getUsableSpace(String dir) {
try {
File file = new File(dir);
if (!file.exists()) {
file = file.getParentFile();
Path file = Path.of(dir);
if (Files.notExists(file)) {
file = file.getParent();
// this is not a disk directory. so just pretend that there is enough space
if (!file.exists()) {
if (Files.notExists(file)) {
return Long.MAX_VALUE;
}
}
FileStore fileStore = Files.getFileStore(file.toPath());
FileStore fileStore = Files.getFileStore(file);
return fileStore.getUsableSpace();
} catch (IOException e) {
throw new SolrException(ErrorCode.SERVER_ERROR, "Could not free disk space", e);
Expand Down Expand Up @@ -1487,7 +1490,7 @@ private void copyTmpConfFiles2Conf(Path tmpconfDir) {
ErrorCode.SERVER_ERROR, "Unable to mkdirs: " + oldPath.getParent(), e);
}
if (Files.exists(oldPath)) {
File oldFile = oldPath.toFile(); // TODO drop this
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()) {
Expand Down Expand Up @@ -1518,12 +1521,14 @@ private void copyTmpConfFiles2Conf(Path tmpconfDir) {
* backup of the old directory is maintained. If the directory move fails, it will try to revert
* back the original tlog directory.
*/
private boolean copyTmpTlogFiles2Tlog(File tmpTlogDir) {
private boolean copyTmpTlogFiles2Tlog(Path tmpTlogDir) {
Path tlogDir =
FileSystems.getDefault().getPath(solrCore.getUpdateHandler().getUpdateLog().getTlogDir());
Path backupTlogDir =
FileSystems.getDefault()
.getPath(tlogDir.getParent().toAbsolutePath().toString(), tmpTlogDir.getName());
.getPath(
tlogDir.getParent().toAbsolutePath().toString(),
tmpTlogDir.getFileName().toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks more complicated than it needs to be. It's highly unusual for anyone to need to refer to FileSystems. Look at the implementation of Path.of(...) which proves we can just use Path.of here.

But hey, copyTmpTlogFiles2Tlog isn't used at all so let's just delete it!


try {
Files.move(tlogDir, backupTlogDir, StandardCopyOption.ATOMIC_MOVE);
Expand All @@ -1534,7 +1539,8 @@ private boolean copyTmpTlogFiles2Tlog(File tmpTlogDir) {

Path src =
FileSystems.getDefault()
.getPath(backupTlogDir.toAbsolutePath().toString(), tmpTlogDir.getName());
.getPath(
backupTlogDir.toAbsolutePath().toString(), tmpTlogDir.getFileName().toString());
try {
Files.move(src, tlogDir, StandardCopyOption.ATOMIC_MOVE);
} catch (IOException e) {
Expand Down Expand Up @@ -1604,9 +1610,9 @@ private Collection<Map<String, Object>> getModifiedConfFiles(
* SecurityException, otherwise returns Throwable preventing deletion (instead of false), for
* additional information.
*/
static Throwable delete(File file) {
static Throwable delete(Path file) {
try {
Files.delete(file.toPath());
Files.delete(file);
return null;
} catch (SecurityException e) {
throw e;
Expand All @@ -1615,9 +1621,9 @@ static Throwable delete(File file) {
}
}

static boolean delTree(File dir) {
static boolean delTree(Path dir) {
try {
org.apache.lucene.util.IOUtils.rm(dir.toPath());
org.apache.lucene.util.IOUtils.rm(dir);
return true;
} catch (IOException e) {
log.warn("Unable to delete directory : {}", dir, e);
Expand Down Expand Up @@ -2046,27 +2052,29 @@ protected class DirectoryFileFetcher extends FileFetcher {
}

private static class LocalFsFile implements FileInterface {
private File copy2Dir;
private Path copy2Dir;

FileChannel fileChannel;
private FileOutputStream fileOutputStream;
File file;
Path file;

LocalFsFile(File dir, String saveAs) throws IOException {
LocalFsFile(Path dir, String saveAs) throws IOException {
this.copy2Dir = dir;

this.file = new File(copy2Dir, saveAs);
this.file = Path.of(copy2Dir.toString(), saveAs);

File parentDir = this.file.getParentFile();
if (!parentDir.exists()) {
if (!parentDir.mkdirs()) {
Path parentDir = this.file.getParent();
if (Files.notExists(parentDir)) {
try {
Files.createDirectories(parentDir);
} catch (Exception e) {
throw new SolrException(
SolrException.ErrorCode.SERVER_ERROR,
"Failed to create (sub)directory for file: " + saveAs);
}
}

this.fileOutputStream = new FileOutputStream(file);
this.fileOutputStream = new FileOutputStream(file.toFile());
this.fileChannel = this.fileOutputStream.getChannel();
}

Expand All @@ -2088,13 +2096,13 @@ public void close() throws Exception {

@Override
public void delete() throws Exception {
Files.delete(file.toPath());
Files.delete(file);
}
}

protected class LocalFsFileFetcher extends FileFetcher {
LocalFsFileFetcher(
File dir,
Path dir,
Map<String, Object> fileDetails,
String saveAs,
String solrParamOutput,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ private void showFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp) {
if (admin == null) { // exception already recorded
return;
}

// TODO SOLR-8282 move to PATH
File adminFile = admin.toFile();
// Make sure the file exists, is readable and is not a hidden file
if (!adminFile.exists()) {
Expand Down Expand Up @@ -252,7 +252,7 @@ private void showFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp) {
params.set(CommonParams.WT, "raw");
req.setParams(params);

ContentStreamBase content = new ContentStreamBase.FileStream(adminFile);
ContentStreamBase content = new ContentStreamBase.FileStream(adminFile.toPath());
content.setContentType(getSafeContentType(req.getParams().get(USE_CONTENT_TYPE)));

rsp.add(RawResponseWriter.CONTENT, content);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@
import static org.apache.solr.common.params.CommonParams.NAME;

import com.codahale.metrics.Gauge;
import java.io.File;
import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.management.ManagementFactory;
import java.lang.management.OperatingSystemMXBean;
import java.lang.management.RuntimeMXBean;
import java.net.InetAddress;
import java.nio.file.Path;
import java.text.DecimalFormat;
import java.text.DecimalFormatSymbols;
import java.util.ArrayList;
Expand Down Expand Up @@ -193,7 +193,7 @@ private SimpleOrderedMap<Object> getCoreInfo(SolrCore core, IndexSchema schema)

// Solr Home
SimpleOrderedMap<Object> dirs = new SimpleOrderedMap<>();
dirs.add("cwd", new File(System.getProperty("user.dir")).getAbsolutePath());
dirs.add("cwd", Path.of(System.getProperty("user.dir")).toAbsolutePath().toString());
dirs.add("instance", core.getInstancePath().toString());
try {
dirs.add("data", core.getDirectoryFactory().normalize(core.getDataDir()));
Expand Down
Loading
Loading