Whirr
  1. Whirr
  2. WHIRR-215

Add builder pattern to addRunUrl() call

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Not a Problem
    • Affects Version/s: 0.3.0
    • Fix Version/s: 0.4.0
    • Component/s: core
    • Labels:
      None

      Description

      If we get optional parameters then the code using varargs gets messy:

      String tarurl = clusterSpec.getConfiguration().getString(
        HBaseConstants.KEY_TARBALL_URL);
      if (tarurl != null) {
        addRunUrl(event, hbaseInstallRunUrl,
          HBaseConstants.PARAM_PROVIDER, clusterSpec.getProvider(),
          HBaseConstants.PARAM_TARBALL_URL, tarurl);
      } else {
        addRunUrl(event, hbaseInstallRunUrl,
          HBaseConstants.PARAM_PROVIDER, clusterSpec.getProvider());
      }
      

      We should extend or add a builder pattern so that one can do something like

      RunUrl ru = RunUrl.create(event)
        .url(hbaseInstallRunUrl)
        .arg(HBaseConstants.PARAM_PROVIDER, clusterSpec.getProvider());
      if (tarUrl != null) {
        st.addArg(HBaseConstants.PARAM_TARBALL_URL, tarurl);
      }
      addRunUrl(ru);
      

      Or similar to http://stackoverflow.com/questions/3838053/mapmaker-design-pattern referring to the Guava MapMaker class.

        Activity

        Hide
        Lars George added a comment -

        Leaving this for later. I have implemented my approach in WHIRR-186. Maybe that can use for others as a template as well. For now this issue is void.

        Show
        Lars George added a comment - Leaving this for later. I have implemented my approach in WHIRR-186 . Maybe that can use for others as a template as well. For now this issue is void.
        Hide
        Tom White added a comment -

        I'm not sure what the reuse opportunity is yet. Should we do it for a couple of services then look for refactoring opportunities?

        Show
        Tom White added a comment - I'm not sure what the reuse opportunity is yet. Should we do it for a couple of services then look for refactoring opportunities?
        Hide
        Lars George added a comment -

        I had a look at the patch and understand the suggested idea. I am just a bit confused if that would need some lower level hook or if you could add this myself somewhere. The issue is reuse I guess, since I would need support in all handlers, so should go into a base CAH I presume? Or would you think this should be supported cross services (each for their own defaults) and enabled per service by for example supplying the name of the default config (or default that to "whirr-<service-name>-default.properties") etc.?

        Show
        Lars George added a comment - I had a look at the patch and understand the suggested idea. I am just a bit confused if that would need some lower level hook or if you could add this myself somewhere. The issue is reuse I guess, since I would need support in all handlers, so should go into a base CAH I presume? Or would you think this should be supported cross services (each for their own defaults) and enabled per service by for example supplying the name of the default config (or default that to "whirr-<service-name>-default.properties") etc.?
        Hide
        Tom White added a comment -

        Have a whirr-hbase-default.properties in src/main/resources, and then build a composite configuration from the default configuration constructed with new PropertiesConfiguration("whirr-hbase-default.properties"). There's some code in WHIRR-55 to do something similar for Hadoop.

        Show
        Tom White added a comment - Have a whirr-hbase-default.properties in src/main/resources, and then build a composite configuration from the default configuration constructed with new PropertiesConfiguration("whirr-hbase-default.properties") . There's some code in WHIRR-55 to do something similar for Hadoop.
        Hide
        Lars George added a comment -

        Ah right, you said so earlier, sorry for missing that. You mentioned a whirr-hbase-default.xml, right? Where would you put those? Simply under main/java/resources? Would I need to call addResources() or so somewhere since the current .properties are parsed by commons.configurations?

        Show
        Lars George added a comment - Ah right, you said so earlier, sorry for missing that. You mentioned a whirr-hbase-default.xml, right? Where would you put those? Simply under main/java/resources? Would I need to call addResources() or so somewhere since the current .properties are parsed by commons.configurations?
        Hide
        Tom White added a comment -

        Alternatively, we could put all the parameter defaults on the client side (i.e. not in the scripts) so they are never null.

        Show
        Tom White added a comment - Alternatively, we could put all the parameter defaults on the client side (i.e. not in the scripts) so they are never null.

          People

          • Assignee:
            Lars George
            Reporter:
            Lars George
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development