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