You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2023/01/17 20:53:17 UTC
[GitHub] [cassandra] josh-mckenzie commented on a diff in pull request #1488: CASSANDRA-17427 Unit test improvements
josh-mckenzie commented on code in PR #1488:
URL: https://github.com/apache/cassandra/pull/1488#discussion_r1072582062
##########
ide/idea/workspace.xml:
##########
@@ -353,4 +353,4 @@
</provider>
</entry>
</component>
-</project>
+</project>
Review Comment:
Why drop whitespace at end of file?
##########
src/java/org/apache/cassandra/io/util/PathUtils.java:
##########
@@ -310,13 +312,39 @@ public static Throwable delete(Path file, Throwable accumulate, @Nullable RateLi
return accumulate;
}
+ private static void deleteRecursiveUsingNixCommand(Path path, boolean quietly)
+ {
+ try
+ {
+ int result = Runtime.getRuntime().exec(new String[]{ "rm", quietly ? "-rf" : "-r", path.toAbsolutePath().toString() }).waitFor();
Review Comment:
nit: build the command and cache in a string locally so you eliminate the potential defect / error of us logging a command that differs from the one we ran.
##########
src/java/org/apache/cassandra/net/MessagingService.java:
##########
@@ -527,11 +530,17 @@ public Future<Void> maybeReconnectWithNewIp(InetAddressAndPort address, InetAddr
*/
public void shutdown()
{
- shutdown(1L, MINUTES, true, true);
+ if (NON_GRACEFUL_SHUTDOWN)
+ // this branch is used in unit-tests when we really never restart a node and shutting down means the end of test
+ shutdown(1L, MINUTES, false, true, false);
+ else
+ shutdown(1L, MINUTES, true, true, true);
}
- public void shutdown(long timeout, TimeUnit units, boolean shutdownGracefully, boolean shutdownExecutors)
+ public void shutdown(long timeout, TimeUnit units, boolean shutdownGracefully, boolean shutdownExecutors, boolean blocking)
Review Comment:
I think this would be cleaner to break into 2 methods, `shutdownGracefully()` and `shutdownAbruptly()` or something, rather than going the multi-bool precondition most of the method is dead on various branches approach we have here.
##########
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:
Not for this patch, but we should create a follow up ticket: we should wrap this in some kind of util method instead of having this if / log at trace pattern peppered throughout the codebase. Something like `FBUtilities.logIfTrace(Logger logger, string msg)`.
##########
src/java/org/apache/cassandra/config/CassandraRelevantProperties.java:
##########
@@ -577,5 +581,4 @@ public boolean isPresent()
{
return System.getProperties().containsKey(key);
}
-}
-
+}
Review Comment:
Why the drop of whitespace at end of file?
##########
src/java/org/apache/cassandra/io/util/PathUtils.java:
##########
@@ -310,13 +312,39 @@ public static Throwable delete(Path file, Throwable accumulate, @Nullable RateLi
return accumulate;
}
+ private static void deleteRecursiveUsingNixCommand(Path path, boolean quietly)
Review Comment:
May want to javadoc why this method exists and why we're using it for future maintainers. Also nit: consider renaming to `deleteRecursiveFast` which would leave us flexibility to switch on platform for faster deletes in the future if (for some bizarre reason) the rm approach ends up slower. Thinking OSX, WSL v2, etc.
##########
src/java/org/apache/cassandra/io/util/PathUtils.java:
##########
@@ -63,14 +63,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();
Review Comment:
Why alias this locally inside PathUtils.java instead of just calling the ``CassandraRelevantProperties.USE_NIX_RECURSIVE_DELETE.getBoolean()`` on each access?
##########
test/distributed/org/apache/cassandra/distributed/impl/AbstractCluster.java:
##########
@@ -184,6 +184,12 @@
private INodeProvisionStrategy.Strategy nodeProvisionStrategy = INodeProvisionStrategy.Strategy.MultipleNetworkInterfaces;
private ShutdownExecutor shutdownExecutor = DEFAULT_SHUTDOWN_EXECUTOR;
+ {
Review Comment:
Is there a reason you went w/the instance initializer vs. static initializer? I'd expect the latter to appropriately nuke the param for everything created inside the JVM context but I could be missing something.
##########
src/java/org/apache/cassandra/net/MessagingService.java:
##########
@@ -233,6 +234,8 @@ public static int getVersionOrdinal(int version)
return ordinal;
}
+ public final static boolean NON_GRACEFUL_SHUTDOWN = Boolean.getBoolean("cassandra.test.messagingService.nonGracefulShutdown");
Review Comment:
Should this be a `CassandraRelevantProperties`?
--
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