iamaleksey commented on code in PR #1944:
URL: https://github.com/apache/cassandra/pull/1944#discussion_r1042162251
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -4573,6 +4566,47 @@ public void run()
}
}
+ /**
+ * Tries to gracefully shutdown MessagingService, if that fails, try
ungracefully.
+ *
+ * Doesn't throw TimeoutException and IOError, only logs
+ */
+ private static void shutdownMessagingServiceWithRetry()
Review Comment:
The method should live inside MessagingService perhaps?
##########
src/java/org/apache/cassandra/net/MessagingService.java:
##########
@@ -452,9 +455,11 @@ public void shutdown(long timeout, TimeUnit units, boolean
shutdownGracefully, b
long deadline = System.nanoTime() + units.toNanos(timeout);
maybeFail(() -> new FutureCombiner(closing).get(timeout, units),
() -> {
+ logger.info("Shutting down inbound executors
({})...", inboundSockets.sockets() != null ? inboundSockets.sockets().size() :
"NULL");
Review Comment:
I'm pretty certain this is not supposed to ever be null, and having a null
check here might give readers the wrong idea. Can we omit the check here?
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -4573,6 +4566,47 @@ public void run()
}
}
+ /**
+ * Tries to gracefully shutdown MessagingService, if that fails, try
ungracefully.
+ *
+ * Doesn't throw TimeoutException and IOError, only logs
+ */
+ private static void shutdownMessagingServiceWithRetry()
+ {
+ try
+ {
+ MessagingService.instance().shutdown();
+ }
+ catch (IOError ioe)
+ {
+ logger.info("Failed to shutdown message service", ioe);
+ }
+ catch (Exception e)
+ {
+ if (e.getCause() instanceof TimeoutException)
+ {
+ try
+ {
+ logger.error("Failed shutting down MessagingService
gracefully, retrying non-gracefully", e);
+ // if we can't shut down gracefully, try hard shutdown
+
MessagingService.instance().shutdown(MessagingService.shutdownTimeoutMinutes,
MINUTES, false, true);
+ }
+ catch (Exception innerEx)
+ {
+ if (innerEx.getCause() instanceof TimeoutException)
+ logger.error("Timed out shutting down
MessagingService", e);
+ else
+ throw innerEx;
+ }
+
Review Comment:
Stray newline :)
##########
src/java/org/apache/cassandra/net/MessagingService.java:
##########
@@ -452,9 +455,11 @@ public void shutdown(long timeout, TimeUnit units, boolean
shutdownGracefully, b
long deadline = System.nanoTime() + units.toNanos(timeout);
maybeFail(() -> new FutureCombiner(closing).get(timeout, units),
() -> {
+ logger.info("Shutting down inbound executors
({})...", inboundSockets.sockets() != null ? inboundSockets.sockets().size() :
"NULL");
Review Comment:
Do we want to have the same logging in the non-graceful branch? And specify
in the message if graceful or not perhaps?
##########
src/java/org/apache/cassandra/net/MessagingService.java:
##########
@@ -452,9 +455,11 @@ public void shutdown(long timeout, TimeUnit units, boolean
shutdownGracefully, b
long deadline = System.nanoTime() + units.toNanos(timeout);
maybeFail(() -> new FutureCombiner(closing).get(timeout, units),
() -> {
+ logger.info("Shutting down inbound executors
({})...", inboundSockets.sockets() != null ? inboundSockets.sockets().size() :
"NULL");
List<ExecutorService> inboundExecutors = new
ArrayList<>();
inboundSockets.close(synchronizedList(inboundExecutors)::add).get();
ExecutorUtils.awaitTermination(1L, TimeUnit.MINUTES,
inboundExecutors);
+ logger.info("Inbound executors shut down");
Review Comment:
Likewise, do we want to have the same logging in the non-graceful branch?
And specify in the message if graceful or not perhaps?
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -4573,6 +4566,47 @@ public void run()
}
}
+ /**
+ * Tries to gracefully shutdown MessagingService, if that fails, try
ungracefully.
+ *
+ * Doesn't throw TimeoutException and IOError, only logs
+ */
+ private static void shutdownMessagingServiceWithRetry()
+ {
+ try
+ {
+ MessagingService.instance().shutdown();
+ }
+ catch (IOError ioe)
+ {
+ logger.info("Failed to shutdown message service", ioe);
Review Comment:
Presumably should log at error too?
##########
src/java/org/apache/cassandra/net/MessagingService.java:
##########
@@ -452,9 +455,11 @@ public void shutdown(long timeout, TimeUnit units, boolean
shutdownGracefully, b
long deadline = System.nanoTime() + units.toNanos(timeout);
maybeFail(() -> new FutureCombiner(closing).get(timeout, units),
() -> {
+ logger.info("Shutting down inbound executors
({})...", inboundSockets.sockets() != null ? inboundSockets.sockets().size() :
"NULL");
List<ExecutorService> inboundExecutors = new
ArrayList<>();
inboundSockets.close(synchronizedList(inboundExecutors)::add).get();
ExecutorUtils.awaitTermination(1L, TimeUnit.MINUTES,
inboundExecutors);
Review Comment:
Should this still be using the hardcoded 1 minute constant?
--
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]
