Whirr
  1. Whirr
  2. WHIRR-199

Add aliases for short role names like nn, jt, tt, dn, zk

    Details

      Description

      Whirr support the following instance roles: dn, jt, nn, tt, zk, hbase-avroserver, hbase-master, hbase-regionserver, hbase-restserver, hbase-thriftserver and as you can see the naming is inconsistent and sometimes it's hard to guess the meaning.

      Tom White replied on the email list:
      "The shorter names (nn, zk, etc) predate Whirr to when the roles were a part of the group name and had to be kept short to conform to various
      length restrictions. We do things differently now that we have multi-cloud support, so keeping them short is less important, and indeed with a global namespace (since service name is optional) there is even more reason to make them distinctive and unique. So I would suggest the longer, hyphenated form that we adopted for HBase, like hbase-regionserver; so the pattern is <service-name>-<daemon>.

      There's not much harm in keeping the shorter forms around, but we could introduce aliases like hadoop-namenode, zookeeper, etc and
      deprecate the short ones."

      1. WHIRR-199.patch
        19 kB
        Andrei Savu
      2. WHIRR-199.patch
        19 kB
        Andrei Savu
      3. WHIRR-199.patch
        17 kB
        Andrei Savu
      4. WHIRR-199.patch
        11 kB
        Andrei Savu

        Activity

        Hide
        Andrei Savu added a comment -

        Thanks Tom for fixing the checkstyle issue, I'm having some problems running mvn checkstyle:checkstyle on my computer.

        Show
        Andrei Savu added a comment - Thanks Tom for fixing the checkstyle issue, I'm having some problems running mvn checkstyle:checkstyle on my computer.
        Hide
        Tom White added a comment -

        I've just committed this (after fixing a remaining checkstyle issue). Thanks Andrei!

        Show
        Tom White added a comment - I've just committed this (after fixing a remaining checkstyle issue). Thanks Andrei!
        Hide
        Andrei Savu added a comment -

        I have updated the patch. Thanks Tom for reviewing.

        Show
        Andrei Savu added a comment - I have updated the patch. Thanks Tom for reviewing.
        Hide
        Tom White added a comment -

        This is almost there, just a couple of small points:

        In the replaceAliases() method you call "roles.toArray(arrayOfRoles);" without assigning to arrayOfRoles. This should work since arrayOfRoles is large enough to contain roles, but it's better to explicitly re-assign to avoid subtle errors (e.g. if roles changes in size between the two operations). In this case it may be simpler to use a List since you can construct it directly from the set: Lists.newArrayList(roles).

        There are also a few star imports which fail "mvn checkstyle:checkstyle".

        Show
        Tom White added a comment - This is almost there, just a couple of small points: In the replaceAliases() method you call "roles.toArray(arrayOfRoles);" without assigning to arrayOfRoles. This should work since arrayOfRoles is large enough to contain roles, but it's better to explicitly re-assign to avoid subtle errors (e.g. if roles changes in size between the two operations). In this case it may be simpler to use a List since you can construct it directly from the set: Lists.newArrayList(roles). There are also a few star imports which fail "mvn checkstyle:checkstyle".
        Hide
        Andrei Savu added a comment -

        It should be ready. I have been able to successfully run all the integration and unit tests.

        Show
        Andrei Savu added a comment - It should be ready. I have been able to successfully run all the integration and unit tests.
        Hide
        Andrei Savu added a comment -

        I have updated the patch. Not ready yet - hadoop integration tests are failing.

        Show
        Andrei Savu added a comment - I have updated the patch. Not ready yet - hadoop integration tests are failing.
        Hide
        Tom White added a comment -

        Maybe add a comment to this effect, so it doesn't get used as a general aliasing mechanism?

        Show
        Tom White added a comment - Maybe add a comment to this effect, so it doesn't get used as a general aliasing mechanism?
        Hide
        Andrei Savu added a comment -

        Yes, but this code should be removed in the following Whirr releases.

        Show
        Andrei Savu added a comment - Yes, but this code should be removed in the following Whirr releases.
        Hide
        Tom White added a comment -

        This will work, but it means that core has knowledge of Hadoop and ZooKeeper names. Another way would be to have trivial subclasses of the ClusterActionHandlers which override getRole(). Perhaps this isn't a problem though, because it's a one off, deprecated and we will remove it in the future. Thoughts?

        Show
        Tom White added a comment - This will work, but it means that core has knowledge of Hadoop and ZooKeeper names. Another way would be to have trivial subclasses of the ClusterActionHandlers which override getRole(). Perhaps this isn't a problem though, because it's a one off, deprecated and we will remove it in the future. Thoughts?
        Hide
        Andrei Savu added a comment -

        First iteration on this, not fully tested yet. Let me know what you think.

        Show
        Andrei Savu added a comment - First iteration on this, not fully tested yet. Let me know what you think.

          People

          • Assignee:
            Andrei Savu
            Reporter:
            Andrei Savu
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development