Code review comment for lp:~jimbaker/pyjuju/new-hook-semantics-5-docs

On Thu, Apr 14, 2011 at 8:59 AM, Gustavo Niemeyer <email address hidden>wrote:

> Review: Needs Fixing
> Very good changes overall, thanks Jim.
>
> We'll need another round:
>
> [1]
>
> 17 +A formula is deployed as a service, potentially multiple times.
> Each
>
> This is very open and perhaps even misleading. Please take this specific
> sentence off. The rest of the paragraph should give better background.
>
>
Reworked intro

>
> [2]
>
> +The `metadata.yaml` file, at the root of the formula definition,
>
> s/definition/directory/
>

Changed

>
>
> [3]
>
> - and `peers` relations (note: that's not yet supported).
> (...)
> - that the relation is required (note: that's not yet supported).
>
> Please add back both unsupported notes. All documentation which is not
> under drafts should reflect what is implemented and may be trusted upon,
> or declare explicitly what is not yet in place.
>
> 130 + At this time, Ensemble does not enforce the `limit` and
> 131 + `optional` constraints, but Ensemble will do so in the
> future.
>
> The note really has to be closer to the explanation. Someone reading about
> the field to tweak a formula will not read the whole text to see what is
> supported or not.
>

Moved description to being closer as suggested

>
>
> [4]
>
> 188 +changes on its underlying machine, and change its own relation
> 189 +settings. Changes to relation settings then trigger further events
> 190 +that hooks can respond to. The following section on hook tools
>
> s/its own relation settings/the relation settings/ (the subject of the
> sentence is "the hook").
>
> Also, please drop the sentence starting at "Changes to relation". You're
> describing what happens from the perspective of an individual unit. For
> an individual unit, changing relation settings will not trigger its own
> hooks to execute again.
>

Fixed

>
>
> [5]
>
> 194 +the desired hook name under the ``hooks/`` directory of the
> formula
> 195 +bundle or directory
>
> s/bundle or directory/directory/
>
> Conceptually, we use the bundle term to reference to a file, so it can't
> have directories.
>

This was from the old text in trunk, so thanks for spotting this so it
doesn't get perpetuated

>
>
> [6]
>
> 195 +bundle or directory. Ensemble then executes the hook without
> command
> 196 +line arguments when the corresponding event occurs.
>
> Please use this in place of the latter sentence:
>
> Ensemble will execute the hook based on its file name when the
> corresponding
> event occurs.
>

Much better

>
> Note that in general we don't have to document the lack of something. If
> it's
> not documented to exist, it's fine to not exist.
>

Ack

>
>
> [7]
>
> 198 +Each hook is optional. Not including a corresponding executable in
> the
>
> s/Each hook is/All hooks are/
>

Changed

>
>
> [8]
>
>
> 213 + * **stop** - Runs when the service unit is stopped. This hook
> will run
> 214 + after any established relations are broken.
>
> Let's take the chance to fix the pre-existing ambiguity in the latter
> sentence:
>
> If relations exist, they will be broken and the respective hooks called
> before
> this hook is called.
>

Replaced as suggested, this is much better

>
>
> [9]
>
> + 1. A remote service unit joins the relation.
>
> Let's complement:
>
> 1. A remote service unit joins the relation, right after the
> relation-joined hook was called.
>
>
Likewise, it's important this execution ordering is presented well since it
can be confusing

> [10]
>
> 229 + This hook is the most common hook for a formula to implement.
> It
> 230 + enables service units to handshake and otherwise negotiate
> their
> 231 + interaction, especially with respect to scaling up and down
> 232 + service deployments.
>
> This doesn't feel very helpful. It's taking assumptions about how the user
> should use the hook in his formula rather than explaining what the hook
> does so users can figure how they can use it. It's also not true that it's
> "specially with respect to scaling up and down", nor that this is the most
> common hook (install would be the most common, I think, but that's not
> important).
>
> Please replace with this simpler version:
>
> This hook enables the formula to modify the service unit state
> (configuration,
> running processes, or anything else) to adapt to the relation settings
> of
> remote units.
>

Agreed on being too general and changed accordingly

>
>
> [11]
>
> 238 + the most sense. Upon that happening, the HAProxy in its
> 239 + ``<relation-name>-relation-changed hook`` can then change its
> own
>
> s/<relation-name>-relation-changed/server-relation-changed/, since this is
> an example.
>
>
Make it more concrete per above

>
> [12]
>
> 242 + This interaction is loosely coupled. The HAProxy formula
> doesn't
> 243 + need to know what services it works with. The other services
> don't
> 244 + need to know they're working with HAProxy. They simply need to
> 245 + agree on a common set of relation settings that allow them to
> be
> 246 + integrated.
>
> This is within the documentation of relation-changed hook, so it's not a
> good
> place to define how relations work conceptually. We also need some text to
> explain that in a generic way, rather than introducing the idea with an
> example.
>
> Please remove this, and then put the following text somewhere sensible
> place
> inside the Hooks section:
>
> """
> Note that the coupling between formulas is defined by which settings are
> required and made available to them through the relation hooks, and how
> these
> settings are used. Those conventions is what defines what the relation
> interface really is, and the interface name in the metadata is simply a way
> to refer to them and avoid that incompatible conversations be attempted.
> Keep that in mind when designing your formulas and relations, since it's
> a good idea to allow the implementation of the formula to change and be
> replaced with alternative versions without changing the relation
> conventions
> in a backwards incompatible way.
> """
>
>
I placed the above text (after some copy editing) in the hooks section. We
may to forward reference this when describing the interface, but it's not
clear to me this is necessary.

> [13]
>
> 261 + * **<relation name>-relation-broken** - Runs when a service is
> 262 + removed from a relation, not just a service unit. This event
>
> This is different from what was previously documented, and I don't think
> it's correct. Please read the original documentation, and maybe the
> implementation as well.
>

Reverted back to the original version, after reviewing the implementation in
UnitLifecycle._process_service_changes

>
>
> [14]
>
> 345 +a non-zero status code. Such changes will then trigger further
> hook
> 346 +execution, through the **<relation name>-relation-changed** hook.
> This
> 347 +mechanism enables a general handshaking mechanism for service
> units to
> 348 +coordinate.
>
> s/hook execution/hook execution in the remote unit(s)/
>

Fixed - this makes the action much clearer

>
> s/handshaking/communication/
>

Changed