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

Reply via email to