You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "mlbiscoc (via GitHub)" <gi...@apache.org> on 2024/12/13 15:07:14 UTC

[PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

mlbiscoc opened a new pull request, #2907:
URL: https://github.com/apache/solr/pull/2907

   https://issues.apache.org/jira/browse/SOLR-17548
   
   # Description
   
   Move from `File` to the more modern `java.nio.file.Path`
   
   # Solution
   
   Tried to find all of the public constructors/methods where `File` is being used or returned as a type and do it's conversion to `Path` instead. If the conversion was simple, also changed the `File` methods being used to its equivalent `Path` methods to stop using the `toFile()` call.
   
   There were some places where it required a much larger rework which felt at of scope of this already large number of changes. For those I placed a `// TODO SOLR-8282 move to PATH`
   
   # Tests
   
   Modified most uses of `File` to `Path` and many of the changes revolve around me removing the `toPath()`.
   
   # Checklist
   
   Please review the following and check all that apply:
   
   - [ ] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
   - [ ] I have created a Jira issue and added the issue ID to my pull request title.
   - [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
   - [ ] I have developed this patch against the `main` branch.
   - [ ] I have run `./gradlew check`.
   - [ ] I have added tests for my changes.
   - [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
   


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


Re: [PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2907:
URL: https://github.com/apache/solr/pull/2907#discussion_r1887546435


##########
solr/core/src/java/org/apache/solr/update/UpdateLog.java:
##########


Review Comment:
   nice!



##########
solr/core/src/java/org/apache/solr/util/FileUtils.java:
##########
@@ -55,16 +54,16 @@ public static void copyFile(File src, File destination) throws IOException {
    * @param fullFile the File to be synced to disk
    * @throws IOException if the file could not be synced
    */
-  public static void sync(File fullFile) throws IOException {
-    if (fullFile == null || !fullFile.exists())
+  public static void sync(Path fullFile) throws IOException {

Review Comment:
   now remove the method



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


Re: [PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #2907:
URL: https://github.com/apache/solr/pull/2907


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


Re: [PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on code in PR #2907:
URL: https://github.com/apache/solr/pull/2907#discussion_r1884801769


##########
solr/core/src/test/org/apache/solr/cli/PostToolTest.java:
##########
@@ -303,7 +303,7 @@ public void testDoFilesMode() throws IOException {
   public void testDetectingIfRecursionPossibleInFilesMode() throws IOException {
     PostTool postTool = new PostTool();
     postTool.recursive = 1; // This is the default
-    File dir = getFile("exampledocs");
+    File dir = getFile("exampledocs").toFile();

Review Comment:
   Agreed. I actually had done a lot of this work already across the repo but there were so many tests using `File` and I tried to avoid the conversions or going through `File`, the changes ended up over 800+ lines... So holding those code changes on my Fork for now.
   
   For now, I was planning on keeping the conversions of toFile() and toPath() and we can move out of all of these conversions as best as possible on another PR. Should be easy with a search. I think here I was trying to get as many of the APIs return types and parameters off of File to keep in scope so it was reviewable.



##########
solr/core/src/test/org/apache/solr/search/TestRecovery.java:
##########


Review Comment:
   Read comment above



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


Re: [PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #2907:
URL: https://github.com/apache/solr/pull/2907#issuecomment-2543060012

   Sounds like you have a strong plan for moving forward @mlbiscoc and that's awesome.   Do holler when you are ready for review and commit.    
   


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


Re: [PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on PR #2907:
URL: https://github.com/apache/solr/pull/2907#issuecomment-2546143484

   @dsmiley Addressed your comments above.


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


Re: [PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on PR #2907:
URL: https://github.com/apache/solr/pull/2907#issuecomment-2546787386

   > now remove the method
   
   Oh duh... Removed!


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


Re: [PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #2907:
URL: https://github.com/apache/solr/pull/2907#discussion_r1884509931


##########
solr/core/src/test/org/apache/solr/cli/PostToolTest.java:
##########
@@ -294,7 +294,7 @@ public void testDoFilesMode() throws IOException {
     postTool.recursive = 0;
     postTool.dryRun = true;
     postTool.solrUpdateUrl = URI.create("http://localhost:8983/solr/fake/update");
-    File dir = getFile("exampledocs");
+    File dir = getFile("exampledocs").toFile();

Review Comment:
   Since we just do a toString, maybe we can simplify this line???



##########
solr/core/src/java/org/apache/solr/cloud/SolrZkServer.java:
##########
@@ -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();

Review Comment:
   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()`??



##########
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"))

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



##########
solr/modules/scripting/src/test/org/apache/solr/scripting/update/ScriptUpdateProcessorFactoryTest.java:
##########
@@ -44,7 +44,7 @@ public static void beforeClass() throws Exception {
     initCore(
         "solrconfig-script-updateprocessor.xml",
         "schema.xml",
-        getFile("scripting/solr").getAbsolutePath());
+        getFile("scripting/solr").toAbsolutePath().toString());

Review Comment:
   Maybe better to change `initCore`?



##########
solr/core/src/test/org/apache/solr/cli/PostToolTest.java:
##########
@@ -294,7 +294,7 @@ public void testDoFilesMode() throws IOException {
     postTool.recursive = 0;
     postTool.dryRun = true;
     postTool.solrUpdateUrl = URI.create("http://localhost:8983/solr/fake/update");
-    File dir = getFile("exampledocs");
+    File dir = getFile("exampledocs").toFile();

Review Comment:
   matbe change postFiles to take in a list of paths?



##########
solr/core/src/test/org/apache/solr/search/TestRecovery.java:
##########


Review Comment:
   This feels like maybe an opportunity to change from using either Strings and Files to just path and avoid some conversions??



##########
solr/core/src/test/org/apache/solr/cli/PostToolTest.java:
##########
@@ -303,7 +303,7 @@ public void testDoFilesMode() throws IOException {
   public void testDetectingIfRecursionPossibleInFilesMode() throws IOException {
     PostTool postTool = new PostTool();
     postTool.recursive = 1; // This is the default
-    File dir = getFile("exampledocs");
+    File dir = getFile("exampledocs").toFile();

Review Comment:
   likewise, maybe we just make strings and avoid going thoruh the file type?



##########
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

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



##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientRetryTest.java:
##########
@@ -41,7 +41,6 @@ public static void setupCluster() throws Exception {
         .addConfig(
             "conf",
             getFile("solrj")
-                .toPath()

Review Comment:
   Sweet!



##########
solr/cross-dc-manager/src/test/org/apache/solr/crossdc/manager/RetryQueueIntegrationTest.java:
##########
@@ -157,7 +157,7 @@ private static MiniSolrCloudCluster startCluster(
 
     // new SolrCloudTestCase.Builder(1, baseDir).addConfig("conf",
     // getFile("configs/cloud-minimal/conf").toPath()).configure();
-    cluster.uploadConfigSet(getFile("configs/cloud-minimal/conf").toPath(), "conf");
+    cluster.uploadConfigSet(getFile("configs/cloud-minimal/conf"), "conf");

Review Comment:
   Love it!   



##########
solr/core/src/test/org/apache/solr/cli/PostToolTest.java:
##########
@@ -303,7 +303,7 @@ public void testDoFilesMode() throws IOException {
   public void testDetectingIfRecursionPossibleInFilesMode() throws IOException {
     PostTool postTool = new PostTool();
     postTool.recursive = 1; // This is the default
-    File dir = getFile("exampledocs");
+    File dir = getFile("exampledocs").toFile();

Review Comment:
   Or stay as a Path?



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


Re: [PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on PR #2907:
URL: https://github.com/apache/solr/pull/2907#issuecomment-2543112752

   > Sounds like you have a strong plan for moving forward @mlbiscoc and that's awesome. Do holler when you are ready for review and commit.
   
   This should be ready for review unless you want me to bring in those other changes but was going for that in a separate PR


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


Re: [PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2907:
URL: https://github.com/apache/solr/pull/2907#discussion_r1885858097


##########
solr/core/src/java/org/apache/solr/spelling/suggest/fst/AnalyzingInfixLookupFactory.java:
##########
@@ -108,7 +109,7 @@ public Lookup create(NamedList<?> params, SolrCore core) {
 
     try {
       return new AnalyzingInfixSuggester(
-          FSDirectory.open(new File(indexPath).toPath()),

Review Comment:
   Wow that line of code was sad.



##########
solr/core/src/java/org/apache/solr/util/FileUtils.java:
##########
@@ -37,14 +36,14 @@ public class FileUtils {
    * If path is absolute, then a File for that path is returned; if it's not absolute, then a File
    * is returned using "path" as a child of "base")
    */
-  public static File resolvePath(File base, String path) {
-    File r = new File(path);
-    return r.isAbsolute() ? r : new File(base, path);
+  public static Path resolvePath(Path base, String path) {
+    Path r = Path.of(path);
+    return r.isAbsolute() ? r : Path.of(base.toString(), path);
   }
 
-  public static void copyFile(File src, File destination) throws IOException {
-    try (FileChannel in = new FileInputStream(src).getChannel();
-        FileChannel out = new FileOutputStream(destination).getChannel()) {
+  public static void copyFile(Path src, Path destination) throws IOException {

Review Comment:
   The only reason this method exists, is because the inputs were File and not Path.  See Files.copy(path, path).  So at this point, you can just write that one-liner then inline this method (IntelliJ will do for you).  IntelliJ kicks but at refactoring; perhaps Eclipse does fine too; I dunno.



##########
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##########
@@ -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());

Review Comment:
   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!



##########
solr/modules/analysis-extras/src/test/org/apache/solr/schema/TestICUCollationFieldOptions.java:
##########
@@ -26,7 +26,7 @@ public class TestICUCollationFieldOptions extends SolrTestCaseJ4 {
   @BeforeClass
   public static void beforeClass() throws Exception {
     File testHome = createTempDir().toFile();
-    FileUtils.copyDirectory(getFile("analysis-extras/solr"), testHome);
+    FileUtils.copyDirectory(getFile("analysis-extras/solr").toFile(), testHome);

Review Comment:
   (same as my last comment)



##########
solr/core/src/java/org/apache/solr/rest/ManagedResourceStorage.java:
##########
@@ -172,16 +172,23 @@ public void configure(SolrResourceLoader loader, NamedList<String> initArgs)
         throw new IllegalArgumentException(
             "Required configuration parameter '" + STORAGE_DIR_INIT_ARG + "' not provided!");
 
-      File dir = new File(storageDirArg);
-      if (!dir.isDirectory()) dir.mkdirs();
+      Path dir = Path.of(storageDirArg);
+      if (!Files.isDirectory(dir)) {
+        try {
+          Files.createDirectories(dir);
+        } catch (IOException e) {
+          throw new SolrException(
+              SolrException.ErrorCode.SERVER_ERROR, "Unable to create storage directory " + dir, e);
+        }
+      }
 
-      storageDir = dir.getAbsolutePath();
+      storageDir = dir.toAbsolutePath().toString();
       log.info("File-based storage initialized to use dir: {}", storageDir);
     }
 
     @Override
     public boolean exists(String storedResourceId) throws IOException {
-      return (new File(storageDir, storedResourceId)).exists();
+      return (Files.exists(Path.of(storageDir, storedResourceId)));

Review Comment:
   there's a needless parenthesis here



##########
solr/core/src/java/org/apache/solr/core/StandardDirectoryFactory.java:
##########
@@ -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());

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



##########
solr/core/src/java/org/apache/solr/util/FileUtils.java:
##########
@@ -55,16 +54,16 @@ public static void copyFile(File src, File destination) throws IOException {
    * @param fullFile the File to be synced to disk
    * @throws IOException if the file could not be synced
    */
-  public static void sync(File fullFile) throws IOException {
-    if (fullFile == null || !fullFile.exists())
+  public static void sync(Path fullFile) throws IOException {

Review Comment:
   Use of RandomAccessFile here is super suspicious to me.  So I see the javadocs of this method show this is copied code.  I went and took a look and I now see that Lucene has a utility that is exposed: org.apache.lucene.util.IOUtils#fsync. So we don't need this method anymore.



##########
solr/core/src/test/org/apache/solr/util/FileUtilsTest.java:
##########
@@ -25,11 +25,11 @@ public class FileUtilsTest extends SolrTestCase {
 
   @Test
   public void testResolve() {

Review Comment:
   FileUtils.resolvePath exists only because of the File API.  We don't need it; don't need to test it either.



##########
solr/core/src/java/org/apache/solr/update/UpdateLog.java:
##########
@@ -731,11 +730,11 @@ private boolean updateFromOldTlogs(UpdateCommand cmd) {
     return (cmd.getFlags() & UpdateCommand.REPLAY) != 0 && state == State.REPLAYING;
   }
 
-  public String[] getLogList(File directory) {
+  public String[] getLogList(Path directory) {
     final String prefix = TLOG_NAME + '.';
-    String[] names = directory.list((dir, name) -> name.startsWith(prefix));
+    String[] names = directory.toFile().list((dir, name) -> name.startsWith(prefix));

Review Comment:
   Please don't call toFile



##########
solr/core/src/test/org/apache/solr/schema/ExternalFileFieldSortTest.java:
##########
@@ -26,7 +26,7 @@
 public class ExternalFileFieldSortTest extends SolrTestCaseJ4 {
 
   static void updateExternalFile() throws IOException {
-    final String testHome = SolrTestCaseJ4.getFile("solr/collection1").getParent();
+    final String testHome = SolrTestCaseJ4.getFile("solr/collection1").getParent().toString();
     String filename = "external_eff";
     Files.copy(Path.of(testHome, filename), Path.of(h.getCore().getDataDir(), filename));

Review Comment:
   testHome needn't be of type String; let it be Path.  In the line below, call testHome.resolve(filename).
   Part of the point of Path is that it's easier (than File) to use the typed variants (Path) instead of relatively untyped String.



##########
solr/modules/analysis-extras/src/test/org/apache/solr/analysis/TestFoldingMultitermExtrasQuery.java:
##########
@@ -33,7 +33,7 @@ public String getCoreName() {
   @BeforeClass
   public static void beforeTests() throws Exception {
     File testHome = createTempDir().toFile();
-    FileUtils.copyDirectory(getFile("analysis-extras/solr"), testHome);
+    FileUtils.copyDirectory(getFile("analysis-extras/solr").toFile(), testHome);

Review Comment:
   Skip File conversion and use PathUtils.copyDirectory



##########
solr/solrj/src/test/org/apache/solr/client/solrj/request/TestConfigSetAdminRequest.java:
##########
@@ -41,7 +41,7 @@ public void testUpload() throws Exception {
     upload.setConfigSetName("name");
     verifyException(upload, "There must be a ContentStream");
 
-    upload.setUploadFile(tmpFile, "application/zip");
+    upload.setUploadFile(tmpFile.toPath(), "application/zip");

Review Comment:
   simply declare tmpFile as Path without conversion and maybe no further changes



##########
solr/modules/clustering/src/test/org/apache/solr/handler/clustering/ClusteringComponentTest.java:
##########
@@ -56,7 +56,7 @@ public class ClusteringComponentTest extends SolrTestCaseJ4 {
   @BeforeClass
   public static void beforeClass() throws Exception {
     File testHome = createTempDir().toFile();
-    FileUtils.copyDirectory(getFile("clustering/solr"), testHome);
+    FileUtils.copyDirectory(getFile("clustering/solr").toFile(), testHome);

Review Comment:
   same



##########
solr/modules/analysis-extras/src/test/org/apache/solr/update/processor/TestOpenNLPExtractNamedEntitiesUpdateProcessorFactory.java:
##########
@@ -29,7 +29,7 @@ public class TestOpenNLPExtractNamedEntitiesUpdateProcessorFactory extends Updat
   @BeforeClass
   public static void beforeClass() throws Exception {
     File testHome = createTempDir().toFile();
-    FileUtils.copyDirectory(getFile("analysis-extras/solr"), testHome);
+    FileUtils.copyDirectory(getFile("analysis-extras/solr").toFile(), testHome);

Review Comment:
   (same)



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


Re: [PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on code in PR #2907:
URL: https://github.com/apache/solr/pull/2907#discussion_r1884801241


##########
solr/core/src/java/org/apache/solr/cloud/SolrZkServer.java:
##########
@@ -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();

Review Comment:
   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



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


Re: [PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on code in PR #2907:
URL: https://github.com/apache/solr/pull/2907#discussion_r1884801317


##########
solr/core/src/test/org/apache/solr/cli/PostToolTest.java:
##########
@@ -294,7 +294,7 @@ public void testDoFilesMode() throws IOException {
     postTool.recursive = 0;
     postTool.dryRun = true;
     postTool.solrUpdateUrl = URI.create("http://localhost:8983/solr/fake/update");
-    File dir = getFile("exampledocs");
+    File dir = getFile("exampledocs").toFile();

Review Comment:
   Feels out of scope for now but agreed maybe we can simplify here. Unless you feel strongly about it, will add a TODO and I can do on a separate PR.



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


Re: [PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on PR #2907:
URL: https://github.com/apache/solr/pull/2907#issuecomment-2542818507

   > Really excited about this refactoring! I made some comments, mostly around "hey, this is a nice chnage" or "hey, is this an opporutnity" versus "please change this" type comments.
   > 
   > One thing I am curious is how do we know all the conversions are done? I like the addition of the TODO to point out where more refactorings can be done. I guess I am wondering, once we have a lot of `.toString()` or `.toPath()`, how do we go back and remove the excess conversion loops? Also, i wonder if we need to add when this is done some forbiddenapi entries for file? I triggered the CI process... hopefully CI will run on all your other commits.
   
   Thanks for helping review @epugh! I found all imports of `java.io.File` and tried to remove it as much as possible. Mostly tried to fix using File if it was trival, otherwise you'll see the `toFile()` or `toPath() This is a first pass through and looks like tests pass. Second pass would probably be to try and remove `toFile()` or `toPath()` as well as creation of any `new File()` as much as possible. If we can make it a forbidden API, the build will tell us where all of them are being called to continue with clean up.
   
   I found the File api was used so much in Tests, when I was doing conversions the number of lines changes became too much for this PR. I am holding those on my fork for now and hopefully I can maybe push that PR after this one.
   
   


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


Re: [PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on code in PR #2907:
URL: https://github.com/apache/solr/pull/2907#discussion_r1884801824


##########
solr/modules/scripting/src/test/org/apache/solr/scripting/update/ScriptUpdateProcessorFactoryTest.java:
##########
@@ -44,7 +44,7 @@ public static void beforeClass() throws Exception {
     initCore(
         "solrconfig-script-updateprocessor.xml",
         "schema.xml",
-        getFile("scripting/solr").getAbsolutePath());
+        getFile("scripting/solr").toAbsolutePath().toString());

Review Comment:
   Same reply as comment above.



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


Re: [PR] SOLR-17548: Switch all public Java APIs from File to Path [solr]

Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #2907:
URL: https://github.com/apache/solr/pull/2907#issuecomment-2543122319

   Cool...   I'll leave it open for a few days to see what else crops up..  Ping me if we don't merge it by mid week!


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org