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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
