SOLR-17548: Switch all public Java APIs from File to Path (#2907)

* Move public Java APIs from File to Path * Move File objects to toPath() * SolrTestCaseJ4 getFile() returns Path instead * More cleanup on toPath() * Fixed tests from wrong Path conversion logic * Fix S3InstallShardTest * MiniClusterState change to Path completely * Address PR comments * Remove sync in FileUtils * Track change --------- Co-authored-by: Eric Pugh <[email protected]>
apache · Dec 19, 2024 · ae93adc · ae93adc
1 parent 9cef6e3
commit ae93adc
Show file tree

Hide file tree

Showing 115 changed files with 306 additions and 446 deletions.
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
@@ -167,6 +167,8 @@ Other Changes
 * SOLR-17579: Remove unused code and other refactorings in ReplicationHandler and tests.  Removed unused public 
   LOCAL_ACTIVITY_DURING_REPLICATION variable. (Eric Pugh)
 
+* SOLR-17548: Switch all public Java APIs from File to Path. (Matthew Biscocho via Eric Pugh)
+
 ==================  9.8.0 ==================
 New Features
 ---------------------

diff --git a/solr/benchmark/src/java/org/apache/solr/bench/MiniClusterState.java b/solr/benchmark/src/java/org/apache/solr/bench/MiniClusterState.java
@@ -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"))
                 .configure();
       } catch (Exception e) {
         if (Files.exists(miniClusterBaseDir)) {
@@ -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;
       }
     }

diff --git a/solr/core/src/java/org/apache/solr/cloud/SolrZkServer.java b/solr/core/src/java/org/apache/solr/cloud/SolrZkServer.java
@@ -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;
@@ -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;
@@ -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();
   }
 
   /**

diff --git a/solr/core/src/java/org/apache/solr/core/DirectoryFactory.java b/solr/core/src/java/org/apache/solr/core/DirectoryFactory.java
@@ -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
     File dataDir = new File(dataDirPath);
     if (!dataDir.isDirectory()) {
       log.debug(

diff --git a/solr/core/src/java/org/apache/solr/core/SolrCore.java b/solr/core/src/java/org/apache/solr/core/SolrCore.java
@@ -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();

diff --git a/solr/core/src/java/org/apache/solr/core/SolrDeletionPolicy.java b/solr/core/src/java/org/apache/solr/core/SolrDeletionPolicy.java
@@ -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;
@@ -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);
     }

diff --git a/solr/core/src/java/org/apache/solr/core/StandardDirectoryFactory.java b/solr/core/src/java/org/apache/solr/core/StandardDirectoryFactory.java
@@ -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;
@@ -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().normalize().toString());
   }
 
   @Override

diff --git a/solr/core/src/java/org/apache/solr/core/ZkContainer.java b/solr/core/src/java/org/apache/solr/core/ZkContainer.java
@@ -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.concurrent.ExecutorService;
 import java.util.concurrent.TimeoutException;
@@ -96,7 +96,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();
@@ -174,7 +174,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('/'));
   }
 

diff --git a/solr/core/src/java/org/apache/solr/handler/IndexFetcher.java b/solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
@@ -51,11 +51,9 @@
 import java.nio.channels.FileChannel;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.FileStore;
-import java.nio.file.FileSystems;
 import java.nio.file.Files;
 import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
-import java.nio.file.StandardCopyOption;
 import java.text.SimpleDateFormat;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -124,7 +122,6 @@
 import org.apache.solr.security.AllowListUrlChecker;
 import org.apache.solr.update.CommitUpdateCommand;
 import org.apache.solr.update.UpdateShardHandler;
-import org.apache.solr.util.FileUtils;
 import org.apache.solr.util.IndexOutputOutputStream;
 import org.apache.solr.util.RTimer;
 import org.apache.solr.util.RefCounted;
@@ -418,7 +415,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
@@ -799,8 +796,9 @@ private void cleanup(
       Directory tmpIndexDir,
       Directory indexDir,
       boolean deleteTmpIdxDir,
-      File tmpTlogDir,
-      boolean successfulInstall) {
+      Path tmpTlogDir,
+      boolean successfulInstall)
+      throws IOException {
     try {
       if (!successfulInstall) {
         try {
@@ -1068,6 +1066,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();
@@ -1079,7 +1078,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));
@@ -1089,7 +1089,7 @@ private void downloadConfFiles(
       terminateAndWaitFsyncService();
       copyTmpConfFiles2Conf(tmpConfPath);
     } finally {
-      delTree(tmpconfDir);
+      delTree(tmpconfDir.toPath());
     }
   }
 
@@ -1156,6 +1156,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
@@ -1203,15 +1204,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);
@@ -1490,7 +1491,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()) {
@@ -1516,49 +1517,6 @@ private void copyTmpConfFiles2Conf(Path tmpconfDir) {
     }
   }
 
-  /**
-   * The tlog files are moved from the tmp dir to the tlog dir as an atomic filesystem operation. A
-   * backup of the old directory is maintained. If the directory move fails, it will try to revert
-   * the original tlog directory.
-   */
-  private boolean copyTmpTlogFiles2Tlog(File tmpTlogDir) {
-    Path tlogDir =
-        FileSystems.getDefault().getPath(solrCore.getUpdateHandler().getUpdateLog().getTlogDir());
-    Path backupTlogDir =
-        FileSystems.getDefault()
-            .getPath(tlogDir.getParent().toAbsolutePath().toString(), tmpTlogDir.getName());
-
-    try {
-      Files.move(tlogDir, backupTlogDir, StandardCopyOption.ATOMIC_MOVE);
-    } catch (IOException e) {
-      log.error("Unable to rename: {} to: {}", tlogDir, backupTlogDir, e);
-      return false;
-    }
-
-    Path src =
-        FileSystems.getDefault()
-            .getPath(backupTlogDir.toAbsolutePath().toString(), tmpTlogDir.getName());
-    try {
-      Files.move(src, tlogDir, StandardCopyOption.ATOMIC_MOVE);
-    } catch (IOException e) {
-      log.error("Unable to rename: {} to: {}", src, tlogDir, e);
-
-      // In case of error, try to revert the original tlog directory
-      try {
-        Files.move(backupTlogDir, tlogDir, StandardCopyOption.ATOMIC_MOVE);
-      } catch (IOException e2) {
-        // bad, we were not able to revert the original tlog directory
-        throw new SolrException(
-            SolrException.ErrorCode.SERVER_ERROR,
-            "Unable to rename: " + backupTlogDir + " to: " + tlogDir);
-      }
-
-      return false;
-    }
-
-    return true;
-  }
-
   private String getDateAsStr(Date d) {
     return new SimpleDateFormat(SnapShooter.DATE_FMT, Locale.ROOT).format(d);
   }
@@ -1601,9 +1559,9 @@ private Collection<Map<String, Object>> getModifiedConfFiles(
     return nameVsFile.isEmpty() ? Collections.emptyList() : nameVsFile.values();
   }
 
-  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);
@@ -2034,28 +1992,29 @@ private static class LocalFsFile implements FileInterface {
 
     FileChannel fileChannel;
     private FileOutputStream fileOutputStream;
-    File file;
+    Path file;
 
-    LocalFsFile(File dir, String saveAs) throws IOException {
+    LocalFsFile(Path dir, String saveAs) throws IOException {
+      this.file = dir.resolve(saveAs);
 
-      this.file = new File(dir, 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();
     }
 
     @Override
     public void sync() throws IOException {
-      FileUtils.sync(file);
+      org.apache.lucene.util.IOUtils.fsync(file, false);
     }
 
     @Override
@@ -2071,13 +2030,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,