cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Stefania (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-12461) Add hooks to StorageService shutdown
Date Wed, 28 Sep 2016 03:49:20 GMT

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

Mime
View raw message