Cassandra
  1. Cassandra
  2. CASSANDRA-4998

Add hook for modifying configuration after cassandra.yaml is loaded

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      When embedding Cassandra in an application, allow programmatic changes to Cassandra's config by using a hook to modify the configuration after the YAML is loaded.

      For this to work, a new YAML configuration needs to be added: after_load_hook_class. If defined, it would be the FQN of a class that implements IAfterLoadHook. When DatabaseDescriptor calls loadYAML(), before any processing of the Config happens, the after_load_hook_class is checked and called if exists. Then processing of the config happens as normal.

        Activity

        Hide
        Michael Guymon added a comment -

        Patch adds IAfterLoadHook and adds after_load_hook_class to Config. DatabaseDescriptor is updated to check and call after_load_hook_class after loading cassandra.yaml. DatabaseDescriptor.loadConfFromYaml(input) was added to make the hook testable without reloading DatabaseDescriptor's Config.

        Show
        Michael Guymon added a comment - Patch adds IAfterLoadHook and adds after_load_hook_class to Config. DatabaseDescriptor is updated to check and call after_load_hook_class after loading cassandra.yaml. DatabaseDescriptor.loadConfFromYaml(input) was added to make the hook testable without reloading DatabaseDescriptor's Config.
        Hide
        Michael Guymon added a comment -

        Updated to remove the need extra cassandra.yaml for the tests

        Show
        Michael Guymon added a comment - Updated to remove the need extra cassandra.yaml for the tests
        Hide
        Jonathan Ellis added a comment -

        What problem is this trying to solve? We're not really interested in targetting the embedded, single server application domaion.

        Show
        Jonathan Ellis added a comment - What problem is this trying to solve? We're not really interested in targetting the embedded, single server application domaion.
        Hide
        Michael Guymon added a comment -

        Correct me if I am wrong, but embedding Cassandra does not prevent it from being in a cluster.

        The goal I am trying to get at is to allow runtime configuration of Cassandra when it inits, instead of only relying on the cassandra.yaml. This will allow Cassandra to be embedded with other services and be part of a standardize configuration process.

        This patch is not very invasive, just adds the option for a callback when the cassandra.yaml loads.

        One of the added benefits of this patch is it make testing easier. By using the callback hook to toggle settings, the conditional processing in DatabaseDescriptor can be asserted. It should allow tests that use an embedded Cassandra to use the callback instead of generating a cassandra.yaml on the fly based on a template.

        Show
        Michael Guymon added a comment - Correct me if I am wrong, but embedding Cassandra does not prevent it from being in a cluster. The goal I am trying to get at is to allow runtime configuration of Cassandra when it inits, instead of only relying on the cassandra.yaml. This will allow Cassandra to be embedded with other services and be part of a standardize configuration process. This patch is not very invasive, just adds the option for a callback when the cassandra.yaml loads. One of the added benefits of this patch is it make testing easier. By using the callback hook to toggle settings, the conditional processing in DatabaseDescriptor can be asserted. It should allow tests that use an embedded Cassandra to use the callback instead of generating a cassandra.yaml on the fly based on a template.
        Hide
        Jonathan Ellis added a comment -

        I'm definitely not a fan of "let's override things programatically instead of just updating the yaml;" that is a great way to shoot yourself in the foot.

        Will let Sylvain weigh in on if this is useful enough for dtests to include anyway.

        Show
        Jonathan Ellis added a comment - I'm definitely not a fan of "let's override things programatically instead of just updating the yaml;" that is a great way to shoot yourself in the foot. Will let Sylvain weigh in on if this is useful enough for dtests to include anyway.
        Hide
        Brandon Williams added a comment -

        It's worth noting that you can call Config.setLoadYaml(false) to prevent loading of the yaml, as BulkRecordWriter and the bulk loader do.

        Show
        Brandon Williams added a comment - It's worth noting that you can call Config.setLoadYaml(false) to prevent loading of the yaml, as BulkRecordWriter and the bulk loader do.
        Hide
        Michael Guymon added a comment -

        I should bring up, the scenario I am attempting, the cassandra.yaml would be package and loaded from the classpath, not exposed directly. There is no foot violence by having to manage multiple configs.

        That being said, this patch would not affect the current way Cassandra runs. Only if someone rolls their own hook, updates the start up of Cassandra so it loads into the classpath, and then explicitly sets it in the cassandra.yaml.

        Show
        Michael Guymon added a comment - I should bring up, the scenario I am attempting, the cassandra.yaml would be package and loaded from the classpath, not exposed directly. There is no foot violence by having to manage multiple configs. That being said, this patch would not affect the current way Cassandra runs. Only if someone rolls their own hook, updates the start up of Cassandra so it loads into the classpath, and then explicitly sets it in the cassandra.yaml.
        Hide
        Sylvain Lebresne added a comment -

        I understand the need to allow configuring the instance by other mean than the yaml, but I'm also not fully convinced by loading the yaml and then run some hook on the result. I'd rather add a mean to directly provide a Config object but in that case skipping the yaml completely. For instance, we could simply replace the current Config.setLoadYaml(boolean) by a Config.useConfig(Config), we would completely remove the need for the yaml, which sounds more generally useful to me. On top of which we could add a simple interface

        interface ConfigurationLoader {
          Config loadConfig();
        }
        

        and we'd add a runtime flag so that one could add -Dcassandra.config.loader=<myLoaderClass>.

        Show
        Sylvain Lebresne added a comment - I understand the need to allow configuring the instance by other mean than the yaml, but I'm also not fully convinced by loading the yaml and then run some hook on the result. I'd rather add a mean to directly provide a Config object but in that case skipping the yaml completely. For instance, we could simply replace the current Config.setLoadYaml(boolean) by a Config.useConfig(Config), we would completely remove the need for the yaml, which sounds more generally useful to me. On top of which we could add a simple interface interface ConfigurationLoader { Config loadConfig(); } and we'd add a runtime flag so that one could add -Dcassandra.config.loader=<myLoaderClass>.
        Hide
        Michael Guymon added a comment -

        That sounds better to me as well. If you like, I can roll another patch + ticket based on this?

        Show
        Michael Guymon added a comment - That sounds better to me as well. If you like, I can roll another patch + ticket based on this?
        Hide
        Michael Guymon added a comment -

        New patch based on feed back CASSANDRA-5045

        Show
        Michael Guymon added a comment - New patch based on feed back CASSANDRA-5045
        Hide
        Sylvain Lebresne added a comment -

        Closing that one in favor of CASSANDRA-5045 then.

        Show
        Sylvain Lebresne added a comment - Closing that one in favor of CASSANDRA-5045 then.

          People

          • Assignee:
            Unassigned
            Reporter:
            Michael Guymon
            Reviewer:
            Sylvain Lebresne
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development