Cassandra
  1. Cassandra
  2. CASSANDRA-4479

JMX attribute setters not consistent with cassandra.yaml

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Fix Version/s: 1.2.0 beta 1
    • Component/s: None
    • Labels:
      None

      Description

      If a setting is configurable both via cassandra.yaml and JMX, the two should be consistent. If that doesn't hold, then the JMX setter can't be trusted.

      Here I present the example of phi_convict_threshold.

      I'm trying to set phi_convict_threshold via JMX, which sets FailureDetector.phiConvictThreshold_, but this doesn't update Config.phi_convict_threshold, which gets its value from cassandra.yaml when starting up.

      Some places, such as FailureDetector.interpret(InetAddress), use FailureDetector.phiConvictThreshold_; others, such as AntiEntropyService.line 813 in cassandra-1.1.2, use Config.phi_convict_threshold:

                  // We want a higher confidence in the failure detection than usual because failing a repair wrongly has a high cost.
                  if (phi < 2 * DatabaseDescriptor.getPhiConvictThreshold())
                      return;
      

      where DatabaseDescriptor.getPhiConvictThreshold() returns Conf.phi_convict_threshold.

      So, it looks like there are cases where a value is stored in multiple places, and setting the value via JMX doesn't set all of them. I'd say there should only be a single place where a configuration parameter is stored, and that single field:

      • should read in the value from cassandra.yaml, optionally falling back to a sane default
      • should be the field that the JMX attribute reads and sets, and
      • any place that needs the current global setting should get it from that field. However, there could be cases where you read in a global value at the start of a task and keep that value locally until the end of the task.

      Also, anything settable via JMX should be volatile or set via a synchronized setter, or else according to the Java memory model other threads may be stuck with the old setting.

      So, I'm requesting the following:

      • Setting up guidelines for how to expose a configuration parameter both via cassandra.yaml and JMX, based on what I've mentioned above
      • Going through the list of configuration parameters and fixing any that don't match those guidelines

      I'd also recommend logging any changes to configuration parameters.

      1. trunk-4479.txt
        13 kB
        Chris Merrill

        Activity

        Hide
        Brandon Williams added a comment -

        Committed, thanks!

        Show
        Brandon Williams added a comment - Committed, thanks!
        Hide
        Chris Merrill added a comment -

        I'm looking into this and will submit a patch for consideration.

        Show
        Chris Merrill added a comment - I'm looking into this and will submit a patch for consideration.
        Hide
        Eric Dong added a comment -

        A quick-and-dirty laundry list of MBean setters:

        $ find . -name '*MBean.java' -exec grep 'void set' {} +
        ./src/java/org/apache/cassandra/concurrent/JMXConfigurableThreadPoolExecutorMBean.java:    void setCorePoolSize(int n);
        ./src/java/org/apache/cassandra/db/ColumnFamilyStoreMBean.java:    public void setMinimumCompactionThreshold(int threshold);
        ./src/java/org/apache/cassandra/db/ColumnFamilyStoreMBean.java:    public void setMaximumCompactionThreshold(int threshold);
        ./src/java/org/apache/cassandra/db/ColumnFamilyStoreMBean.java:    public void setCompactionStrategyClass(String className) throws ConfigurationException;
        ./src/java/org/apache/cassandra/db/ColumnFamilyStoreMBean.java:    public void setCompressionParameters(Map<String,String> opts) throws ConfigurationException;
        ./src/java/org/apache/cassandra/gms/FailureDetectorMBean.java:    public void setPhiConvictThreshold(int phi);
        ./src/java/org/apache/cassandra/service/CacheServiceMBean.java:    public void setRowCacheSavePeriodInSeconds(int rcspis);
        ./src/java/org/apache/cassandra/service/CacheServiceMBean.java:    public void setKeyCacheSavePeriodInSeconds(int kcspis);
        ./src/java/org/apache/cassandra/service/CacheServiceMBean.java:    public void setRowCacheCapacityInMB(long capacity);
        ./src/java/org/apache/cassandra/service/CacheServiceMBean.java:    public void setKeyCacheCapacityInMB(long capacity);
        ./src/java/org/apache/cassandra/service/StorageProxyMBean.java:    public void setHintedHandoffEnabled(boolean b);
        ./src/java/org/apache/cassandra/service/StorageProxyMBean.java:    public void setMaxHintWindow(int ms);
        ./src/java/org/apache/cassandra/service/StorageProxyMBean.java:    public void setMaxHintsInProgress(int qs);
        ./src/java/org/apache/cassandra/service/StorageProxyMBean.java:    public void setRpcTimeout(Long timeoutInMillis);
        ./src/java/org/apache/cassandra/service/StorageServiceMBean.java:    public void setLog4jLevel(String classQualifier, String level);
        ./src/java/org/apache/cassandra/service/StorageServiceMBean.java:    public void setStreamThroughputMbPerSec(int value);
        ./src/java/org/apache/cassandra/service/StorageServiceMBean.java:    public void setCompactionThroughputMbPerSec(int value);
        ./src/java/org/apache/cassandra/service/StorageServiceMBean.java:    public void setIncrementalBackupsEnabled(boolean value);
        

        DatabaseDescriptor setters; according to ArchitectureInternals, all node configuration parameters should be in here:

        $ grep 'void set' src/java/org/apache/cassandra/config/DatabaseDescriptor.java 
            public static void setPartitioner(IPartitioner newPartitioner)
            public static void setEndpointSnitch(IEndpointSnitch eps)
            public static void setRpcTimeout(Long timeOutInMillis)
            public static void setInMemoryCompactionLimit(int sizeInMB)
            public static void setCompactionThroughputMbPerSec(int value)
            public static void setStreamThroughputOutboundMegabitsPerSec(int value)
            public static void setBroadcastAddress(InetAddress broadcastAdd)
            public static void setDynamicUpdateInterval(Integer dynamicUpdateInterval)
            public static void setDynamicResetInterval(Integer dynamicResetInterval)
            public static void setDynamicBadnessThreshold(Double dynamicBadnessThreshold)
            public static void setIncrementalBackupsEnabled(boolean value)
        
        Show
        Eric Dong added a comment - A quick-and-dirty laundry list of MBean setters: $ find . -name '*MBean.java' -exec grep 'void set' {} + ./src/java/org/apache/cassandra/concurrent/JMXConfigurableThreadPoolExecutorMBean.java: void setCorePoolSize(int n); ./src/java/org/apache/cassandra/db/ColumnFamilyStoreMBean.java: public void setMinimumCompactionThreshold(int threshold); ./src/java/org/apache/cassandra/db/ColumnFamilyStoreMBean.java: public void setMaximumCompactionThreshold(int threshold); ./src/java/org/apache/cassandra/db/ColumnFamilyStoreMBean.java: public void setCompactionStrategyClass(String className) throws ConfigurationException; ./src/java/org/apache/cassandra/db/ColumnFamilyStoreMBean.java: public void setCompressionParameters(Map<String,String> opts) throws ConfigurationException; ./src/java/org/apache/cassandra/gms/FailureDetectorMBean.java: public void setPhiConvictThreshold(int phi); ./src/java/org/apache/cassandra/service/CacheServiceMBean.java: public void setRowCacheSavePeriodInSeconds(int rcspis); ./src/java/org/apache/cassandra/service/CacheServiceMBean.java: public void setKeyCacheSavePeriodInSeconds(int kcspis); ./src/java/org/apache/cassandra/service/CacheServiceMBean.java: public void setRowCacheCapacityInMB(long capacity); ./src/java/org/apache/cassandra/service/CacheServiceMBean.java: public void setKeyCacheCapacityInMB(long capacity); ./src/java/org/apache/cassandra/service/StorageProxyMBean.java: public void setHintedHandoffEnabled(boolean b); ./src/java/org/apache/cassandra/service/StorageProxyMBean.java: public void setMaxHintWindow(int ms); ./src/java/org/apache/cassandra/service/StorageProxyMBean.java: public void setMaxHintsInProgress(int qs); ./src/java/org/apache/cassandra/service/StorageProxyMBean.java: public void setRpcTimeout(Long timeoutInMillis); ./src/java/org/apache/cassandra/service/StorageServiceMBean.java: public void setLog4jLevel(String classQualifier, String level); ./src/java/org/apache/cassandra/service/StorageServiceMBean.java: public void setStreamThroughputMbPerSec(int value); ./src/java/org/apache/cassandra/service/StorageServiceMBean.java: public void setCompactionThroughputMbPerSec(int value); ./src/java/org/apache/cassandra/service/StorageServiceMBean.java: public void setIncrementalBackupsEnabled(boolean value); DatabaseDescriptor setters; according to ArchitectureInternals , all node configuration parameters should be in here: $ grep 'void set' src/java/org/apache/cassandra/config/DatabaseDescriptor.java public static void setPartitioner(IPartitioner newPartitioner) public static void setEndpointSnitch(IEndpointSnitch eps) public static void setRpcTimeout(Long timeOutInMillis) public static void setInMemoryCompactionLimit(int sizeInMB) public static void setCompactionThroughputMbPerSec(int value) public static void setStreamThroughputOutboundMegabitsPerSec(int value) public static void setBroadcastAddress(InetAddress broadcastAdd) public static void setDynamicUpdateInterval(Integer dynamicUpdateInterval) public static void setDynamicResetInterval(Integer dynamicResetInterval) public static void setDynamicBadnessThreshold(Double dynamicBadnessThreshold) public static void setIncrementalBackupsEnabled(boolean value)
        Hide
        Eric Dong added a comment -

        For comparison, rpc_timeout_in_ms is settable through JMX via StorageProxy[MBean], but StorageProxy doesn't have its own rpc_timeout_in_ms field, it calls DatabaseDescriptor.setRpcTimeout(Long), which sets Conf.rpc_timeout_in_ms. However, Conf.rpc_timeout_in_ms is neither volatile nor set via a synchronized method, which is still bad.

        Show
        Eric Dong added a comment - For comparison, rpc_timeout_in_ms is settable through JMX via StorageProxy [MBean] , but StorageProxy doesn't have its own rpc_timeout_in_ms field, it calls DatabaseDescriptor.setRpcTimeout(Long), which sets Conf.rpc_timeout_in_ms. However, Conf.rpc_timeout_in_ms is neither volatile nor set via a synchronized method, which is still bad.

          People

          • Assignee:
            Chris Merrill
            Reporter:
            Eric Dong
            Reviewer:
            Brandon Williams
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development