[ https://issues.apache.org/jira/browse/CASSANDRA-12461?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15528282#comment-15528282 ]
Stefania commented on CASSANDRA-12461: -------------------------------------- I'm definitely very much in favor of what you've done by unifying the drain code, behaviorally we are changing shutdown according to the table above, notably we will also flush tables without durable writes, recycle commitlog segments, and stop compactions. I think this should be the correct behavior, it may make shutdown a bit longer but operators normally run drain before shutdown anyway and so it would be a no-op during shutdown. I'm not sure if we need this in 3.0 as well, the shutdown hooks are a new feature, but running a full drain on shutdown may be very useful when upgrading, in case operators forget to call drain. Here is the review in detail (some points overlap or cancel each other): * there are 3 dtest failures which are not on trunk, {{cql_tests.LWTTester}} but they seem unrelated and they pass locally, it is probably a glitch in dtests since the code was changed 2 days ago, but let's repeat dtests at least two more times. * shall we rename {{inShutdownHook}} to something like {{draining}} or {{drained}}? * there is still one unprotected call to {{logger.warn}} in {{drain()}}, line 4462, and a call in {{runShutdownHooks()}} * let's update the documentation for {{drain()}} * shall we catch any exceptions in {{drain()}} to ensure the post shutdown hooks are run also if there is an exception? * the documentation says that the post shutdown hooks should only be called on final shutdown, not on drain, is this still the case? * here is a proposal for some extra work (feel free to turn it down): refactor {{setMode()}} to accept an optional log level instead of a boolean, when the optional is empty it should not log, so we could call this method also from the shutdown hook and possibly replace {{inShutdownHook}} with {{operationMode}}, provided this becomes volatile. I would also add an override since most of the times it is called with the boolean set to true, so the override would have the logging level set to INFO. * given that {{drain()}} is synchronized, can we not just look at {{inShutdownHook}} (or {{operationMode}}) at the beginning of {{drain()}}, to solve CASSANDRA-12509? Is there anything else I am missing about it? * the {{StorageService}} import is unused in the Enabled*.java files * we could replace {{runShutdownHooks();}} with {{Throwables.perform(null, postShutdownHooks.stream().map(h -> h::run));}}, logging any non-null returned exceptions, if not in final shutdown. This would not only avoid one method implementation, but also make the log call visible in {{drain()}}. > Add hooks to StorageService shutdown > ------------------------------------ > > Key: CASSANDRA-12461 > URL: https://issues.apache.org/jira/browse/CASSANDRA-12461 > Project: Cassandra > Issue Type: Bug > Reporter: Anthony Cozzie > Assignee: Anthony Cozzie > Fix For: 3.x > > Attachments: > 0001-CASSANDRA-12461-add-C-support-for-shutdown-runnables.patch > > > The JVM will usually run shutdown hooks in parallel. This can lead to > synchronization problems between Cassandra, services that depend on it, and > services it depends on. This patch adds some simple support for shutdown > hooks to StorageService. > This should nearly solve CASSANDRA-12011 -- This message was sent by Atlassian JIRA (v6.3.4#6332)