[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #1488: CASSANDRA-17427 Unit test improvements

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

jacek-lewandowski commented on code in PR #1488:
URL: https://github.com/apache/cassandra/pull/1488#discussion_r1082208962


##########
src/java/org/apache/cassandra/io/util/PathUtils.java:
##########
@@ -64,14 +64,16 @@
     private static final Set<StandardOpenOption> READ_WRITE_OPTIONS = unmodifiableSet(EnumSet.of(READ, WRITE, CREATE));
     private static final FileAttribute<?>[] NO_ATTRIBUTES = new FileAttribute[0];
 
+    private static final boolean USE_NIX_RECURSIVE_DELETE = CassandraRelevantProperties.USE_NIX_RECURSIVE_DELETE.getBoolean();
+
     private static final Logger logger = LoggerFactory.getLogger(PathUtils.class);
     private static final NoSpamLogger nospam1m = NoSpamLogger.getLogger(logger, 1, TimeUnit.MINUTES);
 
     private static Consumer<Path> onDeletion = path -> {
         if (StorageService.instance.isDaemonSetupCompleted())
             setDeletionListener(ignore -> {});
-        else
-            logger.info("Deleting file during startup: {}", path);
+        else if (logger.isTraceEnabled())

Review Comment:
   Obviously this is not needed in this case.
   
   However, in general case we should use `if` when:
   ```java
   log.trace("message {}", someHeavyToString());
   ```
   
   if we go with a method like `logIfTrace` we would have to either evaluate that `someHeavyToString()` before going to that method regardless trace is enable or not (which undermine the existence of `if` at all) or we would have to pass a lambda, which is also much heavier than just checking `logger.isTraceEnabled`.
   
   I think the rules should be clear there - unless we do explicit string concatenation (which enforces calling `toString()` for each element) or we call something more than just trivial getters, we should have `if`, otherwise we can skip it because it brings nothing.
   
   On the other hand, `Logger` has methods with signatures:
   ```java
   trace(String msg, Object ... paramas);
   trace(String msg, Throwable t);
   ```
   so basically if we want to pass a throwable we cannot make use of passing string format parameters as objects without the need to early call their `toString()` and actually doing manual string concatenation. 
   
   in particular, there is lack of a method like:
   ```java
   trace(String msg, Throwable t, Object ... params)
   ```
   
   Such a method could be useful in that context.
   



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