new-charm Terracotta

Bug #992572 reported by Robert Ayres
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Juju Charms Collection
Fix Released
Undecided
Unassigned

Bug Description

New Terracotta charm based on Terracotta 3.6.2 - lp:~robert-ayres/charms/precise/terracotta/trunk.

I've successfully tested it on LXC and EC2.

Tags: new-charm
James Page (james-page)
Changed in charms:
assignee: nobody → James Page (james-page)
Revision history for this message
James Page (james-page) wrote :

Hi Robert

I've reviewed your charm - looks pretty good - I was able to get a terracotta cluster up and running using the LXC provider really quickly.

I have two comments:

1) hooks/terracotta - configDefaults function

I would use default-java rather that the explicit openjdk-6 path - the next release of Ubuntu will ship openjdk-7 as default so using this link will mean you get auto-upgraded :-)

2) Source of distribution

I like the way you are validating the checksums on the distribution of terracotta - and the fact that this is configuration.

Around 12 months ago we did some work to package terracotta from the upstream binary distribution:

  https://launchpad.net/~terracotta-ubuntu

This was getting some limited use based on feedback I had from users but it would be a nice complement to this charm and would move the stuff I would expect to see in packaging out of the charm.

Note that this point won't block entry to official charm status - but would be nice to discuss at UDS next week.

3) metadata.yaml - peer relationship

The way you have your peer relationship defined in metadata.yaml works fine (although I'm not sure why):

peers:
  server-array: terracotta-server

However its not formed as per spec:

https://juju.ubuntu.com/docs/charm.html#sample-metadata-yaml-files

So I'm a little concerned that it might break at a later date is something in juju changes - I think this is correct:

peers:
  server-array:
    interface: terracotta-server

Marking 'Incomplete' pending fixup of points 1) and 3).

Cheers

James

Changed in charms:
status: New → Incomplete
assignee: James Page (james-page) → nobody
Revision history for this message
James Page (james-page) wrote :

Also:

jamespage@hendrix:~/src/charms/precise/terracotta$ charm proof
I: relation dso has no hooks

Is the DSO relationship implicit from a client->server perspective? or should the charm provide a port number or suchlike to charms relating to this charm know address and port to configure for access?

Revision history for this message
Robert Ayres (robert-ayres) wrote :

Thanks for the review!

I've committed a change that uses 'default-java' as Java home.

Regarding your two other points:

2) I did look at the Terracotta packages you mention but they were for quite an outdated version of Terracotta. For sure I would have preferred to refresh the current ppa and I'd like to do that in the future if I can.

3) I was using the shorthand for specifying the relationship interface. It says in the Juju docs:

'As a shortcut, if these properties are not defined, and instead a single string value is provided next to the relation name, the string is taken as the interface value'

I can change it if this isn't going to be the case in future Juju releases.

And your other comment:

The server should only really be running on port 9510 and I wasn't sure if we should be explicit setting hostnames in relations being that the client can defer this itself. I can make the hostname explicitly set if you want.

Changed in charms:
status: Incomplete → Fix Committed
Revision history for this message
James Page (james-page) wrote : Re: [Bug 992572] Re: new-charm Terracotta

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 07/05/12 16:47, Robert Ayres wrote:
> Thanks for the review!
>
> I've committed a change that uses 'default-java' as Java home.
>
> Regarding your two other points:
>
> 2) I did look at the Terracotta packages you mention but they were
> for quite an outdated version of Terracotta. For sure I would
> have preferred to refresh the current ppa and I'd like to do that
> in the future if I can.

Great - lets hookup on that during the Q cycle.

> 3) I was using the shorthand for specifying the relationship
> interface. It says in the Juju docs:
>
> 'As a shortcut, if these properties are not defined, and instead a
> single string value is provided next to the relation name, the
> string is taken as the interface value'
>
> I can change it if this isn't going to be the case in future Juju
> releases.

Again I learn something new! If its using a documented feature its fine.

> And your other comment:
>
> The server should only really be running on port 9510 and I wasn't
> sure if we should be explicit setting hostnames in relations being
> that the client can defer this itself. I can make the hostname
> explicitly set if you want.

I would only make the port number an explicit relation parameter but
as you state for this charm it should always run on port 9510 so this
is not really required (I tend to set one anyway as it makes it clear
to anyone reading the hook code what port to use) - juju provides the
rest implicitly so you don't need to set the hostname.

Other than that I'll have another look through in the next day or so
but should be good to promulgate to the store!

Cheers

James

- --
James Page
Ubuntu Core Developer
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBCAAGBQJPqST/AAoJEL/srsug59jDw1oP/0Dzcd9xaPfkVcVIQIRm6U7p
t+tYi49bRIpJL+AKI/s6xOx1hbZPL+ccDNq+yhWX7pIkTsbF52XQ914egnbyF1E1
0ZJcov2beHP/XrXCPQ8VK2JdErOWbUlIvyGXtPDLRHTMSqqhEh+WRLXsTQgJmoru
plnIdQg21OOgI1Hn5gA5TfEBufdJjJjLhdgPeydnb+koEJXlmvOJrNxWs7fbY243
ewQzgJ5IsP/BVdwf6IGlCuQxA59zZIVcIlaJVnSMXPlw9MuhrVSiJBs02BWPQO19
NxKP0hIkUmIV1AOoHb4qj0dg93uvQyGsCqJW8xPD64s56Hn4OKkAy+Q/5ocKColZ
KU9oUHHvUCkFPwo7FGyRYByvVa9dvH/VHyPfuY9CqGvmTWkuYRdpsbT+erwPE2dZ
11sbSaZ9MfMulXH6k8mDQHqa9jmcOmQBSV5SIsf5km3ZWg8CZNULrLaiUwAqXPDO
bXFIqt7+WeTvP5rLasg/bTay91/C96ZbOt6JmsxCkuXIWLZpA/Rnn6yrcA9mWos0
d0hCrfgF3go8tl7Hrs6j6YGR0Tt1x2fekxer0TO3ctHGqZjdCDh7Sa2P8htDOCpB
dKc6ddVWohzHuNTdcAFJspqoOkIR9AaFMi+ilBTR34Jis2uV9Rys+CCThDA5gPv0
kKvHpbx/2Pv4WDt6zsJj
=PavV
-----END PGP SIGNATURE-----

Revision history for this message
James Page (james-page) wrote :

Promulgated!

Thanks for your work!

Changed in charms:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.