[GitHub] [cassandra] dcapwell commented on a diff in pull request #1756: CASSANDRA-17769 4.1 Remove usages of Path#toFile() in the snapshot apparatus

[GitBox <gi...@apache.org>]

dcapwell commented on code in PR #1756:
URL: https://github.com/apache/cassandra/pull/1756#discussion_r930437851


##########
src/java/org/apache/cassandra/db/Directories.java:
##########
@@ -1197,6 +1197,11 @@ public static boolean isSecondaryIndexFolder(File dir)
         return dir.name().startsWith(SECONDARY_INDEX_NAME_SEPARATOR);
     }
 
+    public static boolean isSecondaryIndexFolder(Path dir)
+    {
+        return dir.getFileName().toString().startsWith(SECONDARY_INDEX_NAME_SEPARATOR);

Review Comment:
   `getFileName().toString()` really always hated that pattern =/



##########
src/java/org/apache/cassandra/io/sstable/SSTableHeaderFix.java:
##########
@@ -301,7 +301,7 @@ void processFileOrDirectory(Path path)
               .filter(p -> {
                   try
                   {
-                      return Descriptor.fromFilenameWithComponent(new File(p.toFile())).right.type == Component.Type.DATA;
+                      return Descriptor.fromFilenameWithComponent(new File(p.toFile())).right.type == Component.Type.DATA; // checkstyle: permit this invocation

Review Comment:
   do we need to allow? `org.apache.cassandra.io.util.File#File(java.nio.file.Path)` we can just pass in the `p`?  this compiles for me locally
   
   ```
   return Descriptor.fromFilenameWithComponent(new File(p)).right.type == Component.Type.DATA;
   ```



##########
src/java/org/apache/cassandra/service/snapshot/SnapshotLoader.java:
##########
@@ -80,7 +80,7 @@ public Set<TableSnapshot> loadSnapshots()
         {
             try
             {
-                if (dataDir.toFile().exists())
+                if (Files.exists(dataDir))

Review Comment:
   When switching I always forget if there is a subtle difference with regard to bad disks...
   
   
   `java.nio.file.Files#exists` does
   
   ```
   } catch (IOException x) {
               // does not exist or unable to determine if file exists
               return false;
           }
   ```
   
   and File calls `java.io.UnixFileSystem#getBooleanAttributes0`... which is native... it doesn't say it throws... so if disk is bad... does it fail or return false?



-- 
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: pr-unsubscribe@cassandra.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org