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

Clearly demarcate the user and service provider APIs

    Details

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

      Description

      Currently the user API and the API for writing services are mixed up in org.apache.whirr.service. It would be good to package them more clearly.

      1. WHIRR-245.patch
        100 kB
        Tom White
      2. WHIRR-245.patch
        93 kB
        Tom White
      3. WHIRR-245.patch
        91 kB
        Tom White
      4. WHIRR-245.sh
        3 kB
        Tom White
      5. WHIRR-245.sh
        2 kB
        Tom White

        Activity

        Hide
        Tom White added a comment -

        I've just committed this.

        Show
        Tom White added a comment - I've just committed this.
        Hide
        Tom White added a comment -

        A new patch that fixes all these issues. I will commit it soon.

        Show
        Tom White added a comment - A new patch that fixes all these issues. I will commit it soon.
        Hide
        Andrei Savu added a comment -

        +1 (I had to remove by hand services/../*Service.java and META-INF/services/org.apache.whirr.service.Service for cassandra, hadoop and zookeeper). Unit and ZooKeeper integration tests are passing.

        I've noticed only one small issue: all ClusterController instances are named "service". I believe it's a good idea to change this to something like "controller" but we can do it later.

        Show
        Andrei Savu added a comment - +1 (I had to remove by hand services/../*Service.java and META-INF/services/org.apache.whirr.service.Service for cassandra, hadoop and zookeeper). Unit and ZooKeeper integration tests are passing. I've noticed only one small issue: all ClusterController instances are named "service". I believe it's a good idea to change this to something like "controller" but we can do it later.
        Hide
        Tom White added a comment -

        > Do you think it's a good idea to also remove the classes that extend ClusterController / Service from each service? That code it's not used.

        Yes, it's deprecated too. I've removed it in this patch.

        I ran the integration tests and they passed. I think this is now ready for commit.

        Show
        Tom White added a comment - > Do you think it's a good idea to also remove the classes that extend ClusterController / Service from each service? That code it's not used. Yes, it's deprecated too. I've removed it in this patch. I ran the integration tests and they passed. I think this is now ready for commit.
        Hide
        Andrei Savu added a comment -

        Looks good. Do you think it's a good idea to also remove the classes that extend ClusterController / Service from each service? That code it's not used.

        Show
        Andrei Savu added a comment - Looks good. Do you think it's a good idea to also remove the classes that extend ClusterController / Service from each service? That code it's not used.
        Hide
        Tom White added a comment -

        I've now written a script to do the moves for this change (including moving ClusterAction to org.apache.whirr, but leaving org.apache.whirr.service.jclouds - this can be done in another issue if needed.) To apply, run the script then apply the patch.

        Now that Voldemort is in we don't have any patches for new services waiting, so it would be good to get this change in, as it is pretty wide ranging.

        I'm still running tests, but would appreciate a review.

        Show
        Tom White added a comment - I've now written a script to do the moves for this change (including moving ClusterAction to org.apache.whirr, but leaving org.apache.whirr.service.jclouds - this can be done in another issue if needed.) To apply, run the script then apply the patch. Now that Voldemort is in we don't have any patches for new services waiting, so it would be good to get this change in, as it is pretty wide ranging. I'm still running tests, but would appreciate a review.
        Hide
        Tom White added a comment -

        > Regarding o.a.w.service.actions though, I think actions are better placed in o.a.w.actions as they act on the cluster and on multiple services much like Service now acts on multiple services.

        I agree. I think ClusterAction should be moved to org.apache.whirr for the same reason.

        Show
        Tom White added a comment - > Regarding o.a.w.service.actions though, I think actions are better placed in o.a.w.actions as they act on the cluster and on multiple services much like Service now acts on multiple services. I agree. I think ClusterAction should be moved to org.apache.whirr for the same reason.
        Hide
        David Alves added a comment -

        +1 with one exception.

        I think most suggested changes are great, the codebase is getting a bit disorganized.

        Regarding o.a.w.service.actions though, I think actions are better placed in o.a.w.actions as they act on the cluster and on multiple services much like Service now acts on multiple services.

        Show
        David Alves added a comment - +1 with one exception. I think most suggested changes are great, the codebase is getting a bit disorganized. Regarding o.a.w.service.actions though, I think actions are better placed in o.a.w.actions as they act on the cluster and on multiple services much like Service now acts on multiple services.
        Hide
        Tom White added a comment -

        I suggest that the user-facing API moves to org.apache.whirr, and the API for writing services goes in org.apache.whirr.service and subpackages. This would be a breaking change, so we should try to do this sooner rather than later.

        More specifically:

        • Move the client API (Cluster, ClusterSpec, RolePredicates, Service, ServiceFactory) from the org.apache.whirr.service package to the org.apache.whirr package.
        • Rename Service(Factory) to ClusterController(Factory). Rationale: since WHIRR-117 Service has been able to start one or more services on a cluster, so it's misnamed now; also it's really about interacting with a cluster, not a service.
        • Rename org.apache.whirr.cluster.actions to org.apache.whirr.service.actions so its associated more closely with the classes in org.apache.whirr.service.
        • Move org.apache.whirr.net and org.apache.whirr.ssh to org.apache.whirr.service.util to emphasise that these are utility classes for services.
        • Consider moving classes in org.apache.whirr.service.jclouds to org.apache.whirr.service since the distinction isn't clearcut or particularly useful for service implementors.

        Would org.apache.whirr.service be better named org.apache.whirr.spi to clearly indicate it's a Service Provider Interface? I was tending towards org.apache.whirr.service since the services (Hadoop, HBase, etc) are all under that package.

        Thoughts?

        Show
        Tom White added a comment - I suggest that the user-facing API moves to org.apache.whirr, and the API for writing services goes in org.apache.whirr.service and subpackages. This would be a breaking change, so we should try to do this sooner rather than later. More specifically: Move the client API (Cluster, ClusterSpec, RolePredicates, Service, ServiceFactory) from the org.apache.whirr.service package to the org.apache.whirr package. Rename Service(Factory) to ClusterController(Factory). Rationale: since WHIRR-117 Service has been able to start one or more services on a cluster, so it's misnamed now; also it's really about interacting with a cluster, not a service. Rename org.apache.whirr.cluster.actions to org.apache.whirr.service.actions so its associated more closely with the classes in org.apache.whirr.service. Move org.apache.whirr.net and org.apache.whirr.ssh to org.apache.whirr.service.util to emphasise that these are utility classes for services. Consider moving classes in org.apache.whirr.service.jclouds to org.apache.whirr.service since the distinction isn't clearcut or particularly useful for service implementors. Would org.apache.whirr.service be better named org.apache.whirr.spi to clearly indicate it's a Service Provider Interface? I was tending towards org.apache.whirr.service since the services (Hadoop, HBase, etc) are all under that package. Thoughts?

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development