[jira] [Commented] (CASSANDRA-12179) Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop
[ https://issues.apache.org/jira/browse/CASSANDRA-12179?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15401401#comment-15401401 ] sankalp kohli commented on CASSANDRA-12179: --- LGTM > Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop > --- > > Key: CASSANDRA-12179 > URL: https://issues.apache.org/jira/browse/CASSANDRA-12179 > Project: Cassandra > Issue Type: Improvement >Reporter: sankalp kohli >Assignee: sankalp kohli >Priority: Trivial > Fix For: 3.0.x > > Attachments: CASSANDRA-12179-3.0_v2.txt, CASSANDRA-12179_3.0.txt, > CASSANDRA-12179_3.0_v3.txt > > > Need to expose dynamic_snitch_update_interval_in_ms so that it does not > require a bounce. This is useful for large clusters where we can change this > value and see the impact. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-12179) Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop
[
https://issues.apache.org/jira/browse/CASSANDRA-12179?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15401148#comment-15401148
]
Robert Stupp commented on CASSANDRA-12179:
--
Forgot to mention: Generally this would have to go into trunk since it
introduces a new feature. But on the other side this would fix {{updateSnitch}}
- not sure on this one. Comments from others appreciated.
> Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop
> ---
>
> Key: CASSANDRA-12179
> URL: https://issues.apache.org/jira/browse/CASSANDRA-12179
> Project: Cassandra
> Issue Type: Improvement
>Reporter: sankalp kohli
>Assignee: sankalp kohli
>Priority: Trivial
> Fix For: 3.0.x
>
> Attachments: CASSANDRA-12179-3.0_v2.txt, CASSANDRA-12179_3.0.txt,
> CASSANDRA-12179_3.0_v3.txt
>
>
> Need to expose dynamic_snitch_update_interval_in_ms so that it does not
> require a bounce. This is useful for large clusters where we can change this
> value and see the impact.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-12179) Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop
[
https://issues.apache.org/jira/browse/CASSANDRA-12179?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15401141#comment-15401141
]
Robert Stupp commented on CASSANDRA-12179:
--
Some more comments:
* The {{if}} condition
[here|https://github.com/apache/cassandra/commit/251c8d7499453452604fad1f577450fc979217e3#diff-b76a607445d53f18a98c9df14323c7ddR4245]
is always wrong.
* The logic in {{StorageService.updateSnitch}} would effectively remove the
{{DynamicEndpointSnitch}} mbean since it first instantiates a new dynamic
snitch instance and then unregisters the mbean from the old one (not sure if
that wouldn't cause an exception anyway).
* In c.yaml it is possible to pass in the "abbreviated"/short snitch class
name, but {{updateSnitch}} always expects the full class name. (Not introduced
by this patch)
* The methods {{changeDynamicResetIntervalInMs}} + {{changeBadnessThreshold}} +
{{changeDynamicUpdateIntervalInMs}} can be merged into a single method.
* The logic to restart the tasks in {{DynamicEndpointSnicht}} is fine.
I took the freedom to update the patch in [this
commit|https://github.com/apache/cassandra/commit/feb38d92d5e4bf0176d68c409478c7a21e1f9bf7]
to address the above points and also:
* update the javadoc on the {{updateSnitch}} method
* reuse the {{DatabaseDescriptor.createEndpointSnitch}} method (and change it a
bit) to allow short snitch class names
* merge the change* methods into a single {{applyConfigChanges}} method
* merge {{close}} and {{unregisterMBean}} methods
* fix the {{updateSnitch}} unregister-bbean logic and also refactor it to just
cover the two cases when the snitch is changed and when it is not changed.
WDYT?
(Triggered new CI runs)
> Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop
> ---
>
> Key: CASSANDRA-12179
> URL: https://issues.apache.org/jira/browse/CASSANDRA-12179
> Project: Cassandra
> Issue Type: Improvement
>Reporter: sankalp kohli
>Assignee: sankalp kohli
>Priority: Trivial
> Fix For: 3.0.x
>
> Attachments: CASSANDRA-12179-3.0_v2.txt, CASSANDRA-12179_3.0.txt,
> CASSANDRA-12179_3.0_v3.txt
>
>
> Need to expose dynamic_snitch_update_interval_in_ms so that it does not
> require a bounce. This is useful for large clusters where we can change this
> value and see the impact.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-12179) Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop
[ https://issues.apache.org/jira/browse/CASSANDRA-12179?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15387196#comment-15387196 ] Robert Stupp commented on CASSANDRA-12179: -- CI results: ||trunk|[branch|https://github.com/apache/cassandra/compare/trunk...snazy:9054-split-dd-trunk]|[testall|http://cassci.datastax.com/view/Dev/view/snazy/job/snazy-9054-split-dd-trunk-testall/lastSuccessfulBuild/]|[dtest|http://cassci.datastax.com/view/Dev/view/snazy/job/snazy-9054-split-dd-trunk-dtest/lastSuccessfulBuild/] ||cassandra-3.0|[branch|https://github.com/apache/cassandra/compare/cassandra-3.0...snazy:cassandra-3.0]|[testall|http://cassci.datastax.com/view/Dev/view/snazy/job/snazy-cassandra-3.0-testall/lastSuccessfulBuild/]|[dtest|http://cassci.datastax.com/view/Dev/view/snazy/job/snazy-cassandra-3.0-dtest/lastSuccessfulBuild/] > Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop > --- > > Key: CASSANDRA-12179 > URL: https://issues.apache.org/jira/browse/CASSANDRA-12179 > Project: Cassandra > Issue Type: Improvement >Reporter: sankalp kohli >Assignee: sankalp kohli >Priority: Trivial > Fix For: 3.0.x > > Attachments: CASSANDRA-12179-3.0_v2.txt, CASSANDRA-12179_3.0.txt, > CASSANDRA-12179_3.0_v3.txt > > > Need to expose dynamic_snitch_update_interval_in_ms so that it does not > require a bounce. This is useful for large clusters where we can change this > value and see the impact. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-12179) Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop
[ https://issues.apache.org/jira/browse/CASSANDRA-12179?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15385183#comment-15385183 ] Robert Stupp commented on CASSANDRA-12179: -- Looks good so far. Let me play with that a bit. > Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop > --- > > Key: CASSANDRA-12179 > URL: https://issues.apache.org/jira/browse/CASSANDRA-12179 > Project: Cassandra > Issue Type: Improvement >Reporter: sankalp kohli >Assignee: sankalp kohli >Priority: Trivial > Fix For: 3.0.x > > Attachments: CASSANDRA-12179-3.0_v2.txt, CASSANDRA-12179_3.0.txt, > CASSANDRA-12179_3.0_v3.txt > > > Need to expose dynamic_snitch_update_interval_in_ms so that it does not > require a bounce. This is useful for large clusters where we can change this > value and see the impact. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-12179) Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop
[ https://issues.apache.org/jira/browse/CASSANDRA-12179?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15383310#comment-15383310 ] sankalp kohli commented on CASSANDRA-12179: --- Attaching v3 using update snitch. I also fixed issues in the updatesnitch method. > Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop > --- > > Key: CASSANDRA-12179 > URL: https://issues.apache.org/jira/browse/CASSANDRA-12179 > Project: Cassandra > Issue Type: Improvement >Reporter: sankalp kohli >Assignee: sankalp kohli >Priority: Trivial > Fix For: 3.0.x > > Attachments: CASSANDRA-12179-3.0_v2.txt, CASSANDRA-12179_3.0.txt, > CASSANDRA-12179_3.0_v3.txt > > > Need to expose dynamic_snitch_update_interval_in_ms so that it does not > require a bounce. This is useful for large clusters where we can change this > value and see the impact. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-12179) Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop
[
https://issues.apache.org/jira/browse/CASSANDRA-12179?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15382033#comment-15382033
]
Robert Stupp commented on CASSANDRA-12179:
--
I think the instanceof check is fine - just realised that
{{StorageService.updateSnitch}} exists and does it in a similar but broken way.
* {{updateSnitch}} instantiates a new instance of the snitch but it doesn't
cleanup the old instance properly - i.e. the scheduled tasks for update + reset
of the {{DynamicEndpointSnitch}} keep running forever. Probably worth to rename
{{DynamicEndpointSnitch.unregisterMBean}} to {{tearDown()}} and have it stop
the scheduled tasks.
* The implementation of {{updateSnitch}} wouldn't apply changes to the
parameters for {{DynamicEndpointSnitch}}, because it first instantiates the
snitch and then updates the configs via {{DatabaseDescriptor.set...}}} - but
{{DynamicEndpointSnitch}} reads these values during initialization (so before
the changes are applied).
* There are also a couple of {{null}} checks missing in {{updateSnitch}}.
* Beside that, {{StorageService.updateSnitch()}} also updates the {{snitch}}
field in all instances of {{AbstractReplicationStrategy}} - this might lead to
a race condition in case a keyspace's concurrently created. Unrelated to this
ticket, but I do not see the maybe obvious reason why
{{AbstractReplicationStrategy}} has a field {{snitch}} and does not use the
global setting.
* {{changeDynamicUpdateIntervalInMs}} changes the interval of the update task
but it doesn't change {{UPDATE_INTERVAL_IN_MS}} - this should become a
modifiable instance variable called {{updateIntervalInMs}}.
Sorry for coming up with that this late, but {{updateSnitch}} feels very
related to {{setDynamicUpdateInterval}}. I'd also be fine to let
{{setDynamicUpdateInterval(int dynamicUpdateInterval}} call
{{updateSnitch(null, null, dynamicUpdateInterval, null, null)}} and let
{{updateSnitch}} handle the actual changes.
WDYT?
> Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop
> ---
>
> Key: CASSANDRA-12179
> URL: https://issues.apache.org/jira/browse/CASSANDRA-12179
> Project: Cassandra
> Issue Type: Improvement
>Reporter: sankalp kohli
>Assignee: sankalp kohli
>Priority: Trivial
> Fix For: 3.0.x
>
> Attachments: CASSANDRA-12179-3.0_v2.txt, CASSANDRA-12179_3.0.txt
>
>
> Need to expose dynamic_snitch_update_interval_in_ms so that it does not
> require a bounce. This is useful for large clusters where we can change this
> value and see the impact.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-12179) Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop
[ https://issues.apache.org/jira/browse/CASSANDRA-12179?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15381562#comment-15381562 ] sankalp kohli commented on CASSANDRA-12179: --- Attached v2. I am not sure how I can get access to DynamicEndpointSnitch object without instance of check. > Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop > --- > > Key: CASSANDRA-12179 > URL: https://issues.apache.org/jira/browse/CASSANDRA-12179 > Project: Cassandra > Issue Type: Improvement >Reporter: sankalp kohli >Assignee: sankalp kohli >Priority: Trivial > Fix For: 3.0.x > > Attachments: CASSANDRA-12179-3.0_v2.txt, CASSANDRA-12179_3.0.txt > > > Need to expose dynamic_snitch_update_interval_in_ms so that it does not > require a bounce. This is useful for large clusters where we can change this > value and see the impact. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-12179) Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop
[
https://issues.apache.org/jira/browse/CASSANDRA-12179?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15376038#comment-15376038
]
Robert Stupp commented on CASSANDRA-12179:
--
Can you expose the setting via the the mbean using a getter, too?
I think it's necessary to restart the task setup in the
{{DynamicEndpointSnitch}} c'tor as it's [initialized
here|https://github.com/apache/cassandra/blob/04e7723e552459d4b96cea4b5bfbbc5773b0cd68/src/java/org/apache/cassandra/locator/DynamicEndpointSnitch.java#L91].
> Make DynamicEndpointSnitch dynamic_snitch_update_interval_in_ms a JMX Prop
> ---
>
> Key: CASSANDRA-12179
> URL: https://issues.apache.org/jira/browse/CASSANDRA-12179
> Project: Cassandra
> Issue Type: Improvement
>Reporter: sankalp kohli
>Assignee: sankalp kohli
>Priority: Trivial
> Attachments: CASSANDRA-12179_3.0.txt
>
>
> Need to expose dynamic_snitch_update_interval_in_ms so that it does not
> require a bounce. This is useful for large clusters where we can change this
> value and see the impact.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
