Apache Whirr (retired)
  1. Apache Whirr (retired)
  2. WHIRR-75

Use Commons Configuration to manage cluster specs

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.1.0
    • Component/s: core
    • Labels:
      None

      Description

      Apache Commons Configuration (http://commons.apache.org/configuration/index.html) would be useful to share configuration information between the CLI and the tests.

      1. WHIRR-75.patch
        47 kB
        Tom White
      2. WHIRR-75.patch
        49 kB
        Tom White
      3. WHIRR-75.patch
        49 kB
        Tom White
      4. WHIRR-75.patch
        49 kB
        Tom White
      5. WHIRR-75.patch
        49 kB
        Tom White

        Activity

        Hide
        Tom White added a comment -

        This is quite a big change, but should make Whirr easier to use and more flexible. Summary of changes:

        • Commons Configuration is now used for managing configuration. This is shared between tests and the CLI, which means there are fewer places to change when adding new configuration options.
        • ClusterSpec is now immutable and backed by a configuration object.
        • The CLI is simplified. Service and cluster names are set using options, and it's possible to bundle up all the options into a properties file, like so: whirr launch-cluster --config mycluster.properties. (This makes it easy to define clusters with different properties - e.g. a 3-node ZK cluster vs a 5-node cross-DC cluster.) You can still specify options on the command line, and these take precedence over those in the config.
        • Tests have a properties file to drive them. This makes it easy to run the tests with overridden settings (using -Dconfig=mycluster.properties, I use this to set whirr.client-cidrs, for example), and reduces the amount of code duplication for creating a ClusterSpec object.

        All tests pass.

        Show
        Tom White added a comment - This is quite a big change, but should make Whirr easier to use and more flexible. Summary of changes: Commons Configuration is now used for managing configuration. This is shared between tests and the CLI, which means there are fewer places to change when adding new configuration options. ClusterSpec is now immutable and backed by a configuration object. The CLI is simplified. Service and cluster names are set using options, and it's possible to bundle up all the options into a properties file, like so: whirr launch-cluster --config mycluster.properties. (This makes it easy to define clusters with different properties - e.g. a 3-node ZK cluster vs a 5-node cross-DC cluster.) You can still specify options on the command line, and these take precedence over those in the config. Tests have a properties file to drive them. This makes it easy to run the tests with overridden settings (using -Dconfig=mycluster.properties, I use this to set whirr.client-cidrs, for example), and reduces the amount of code duplication for creating a ClusterSpec object. All tests pass.
        Hide
        Adrian Cole (Inactive) added a comment -

        patch applies clean and unit tests pass.

        The design looks cleaner than before and it looks positive.

        I'd like to see an example of creating the configuration without using properties files.

        for some reason, only hadoop seems to pass integration tests now. perhaps it is transient...

        Show
        Adrian Cole (Inactive) added a comment - patch applies clean and unit tests pass. The design looks cleaner than before and it looks positive. I'd like to see an example of creating the configuration without using properties files. for some reason, only hadoop seems to pass integration tests now. perhaps it is transient...
        Hide
        Tom White added a comment -

        Thanks for taking a look, Adrian.

        > I'd like to see an example of creating the configuration without using properties files.

        See DestroyClusterCommandTest which constructs a PropertiesConfiguration in code, without loading it from a properties file. Is this what you mean?

        > for some reason, only hadoop seems to pass integration tests now. perhaps it is transient...

        Could it be that your originating IP changes between calls? To work around this you could create a properties file called .whirr-test.properties in your home directory containing

        whirr.client-cidrs=0.0.0.0/0
        

        (or a more restrictive CIDR). Then run tests using

        mvn verify -DargLine="-Dwhirr.test.identity=$AWS_ACCESS_KEY_ID -Dwhirr.test.credential=$AWS_SECRET_ACCESS_KEY -Dconfig=.whirr-test.properties"
        
        Show
        Tom White added a comment - Thanks for taking a look, Adrian. > I'd like to see an example of creating the configuration without using properties files. See DestroyClusterCommandTest which constructs a PropertiesConfiguration in code, without loading it from a properties file. Is this what you mean? > for some reason, only hadoop seems to pass integration tests now. perhaps it is transient... Could it be that your originating IP changes between calls? To work around this you could create a properties file called .whirr-test.properties in your home directory containing whirr.client-cidrs=0.0.0.0/0 (or a more restrictive CIDR). Then run tests using mvn verify -DargLine="-Dwhirr.test.identity=$AWS_ACCESS_KEY_ID -Dwhirr.test.credential=$AWS_SECRET_ACCESS_KEY -Dconfig=.whirr-test.properties"
        Hide
        Tom White added a comment -

        There was a bug in the previous patch that meant client CIDRs were being ignored. ZK and Cassandra tests work for me now with this patch.

        Show
        Tom White added a comment - There was a bug in the previous patch that meant client CIDRs were being ignored. ZK and Cassandra tests work for me now with this patch.
        Hide
        Tom White added a comment -

        Adrian noticed that there was a missing trim() on the FirewallSettings.getOriginatingIp() method. New patch to remedy.

        Show
        Tom White added a comment - Adrian noticed that there was a missing trim() on the FirewallSettings.getOriginatingIp() method. New patch to remedy.
        Hide
        Adrian Cole added a comment -

        latest patch tests out fine on cassandra, hadoop, and zookeeper

        Show
        Adrian Cole added a comment - latest patch tests out fine on cassandra, hadoop, and zookeeper
        Hide
        Adrian Cole added a comment -

        thanks for the notes on usage. I would prefer our objects to not require commons configuration. For example, It would be a bit cleaner to have ClusterSpec as a pojo and a utility that allows you to create it from a commons configuration object.

        Show
        Adrian Cole added a comment - thanks for the notes on usage. I would prefer our objects to not require commons configuration. For example, It would be a bit cleaner to have ClusterSpec as a pojo and a utility that allows you to create it from a commons configuration object.
        Hide
        Tom White added a comment -

        New patch which makes ClusterSpec a POJO, and has a static fromConfiguration() factory method. Does this look OK?

        Show
        Tom White added a comment - New patch which makes ClusterSpec a POJO, and has a static fromConfiguration() factory method. Does this look OK?
        Hide
        Adrian Cole (Inactive) added a comment -

        I'd kindof prefer clusterspec to be immutable (ex. all final fields or no setters), but I feel its nit-picky

        I'm happy with how this is

        +1

        Show
        Adrian Cole (Inactive) added a comment - I'd kindof prefer clusterspec to be immutable (ex. all final fields or no setters), but I feel its nit-picky I'm happy with how this is +1
        Hide
        Tom White added a comment -

        I've just committed this.

        Happy to make further changes to ClusterSpec - perhaps we can discuss in another issue?

        Show
        Tom White added a comment - I've just committed this. Happy to make further changes to ClusterSpec - perhaps we can discuss in another issue?

          People

          • Assignee:
            Tom White
            Reporter:
            Tom White
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development