Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(507)

Issue 6405044: add relationUnitsWatcher

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 7 months ago by fwereade
Modified:
7 years, 7 months ago
Reviewers:
mp+115001
Visibility:
Public.

Description

add relationUnitsWatcher ...which expresses changes to a relation from a given unit's perspective, in terms that I believe will be helpful for the unit agent. In particular, the main event type produced has Changed and Departed fields, under the rationale that the UA will itself be keeping track of membership as it does in the python version; and that therefore it is actually more helpful to expect it to pay attention only to -changed and -departed events, and to automatically run -joined hooks *before* running -changed hooks in response to unrecognised units (rather than spearately tracking all 3 kinds of event, and *also* inserting -changed events immediately after -joineds, as is currently done in the python). The intent is to keep this type internal, and use it only within a (tentatively-named) RelationUnit type, which (1) exposes the relationUnitsWatcher's Changes channel and (2) maintains a Pinger signalling its own presence in the relation; I'm expecting it to look something like: func (*Unit) Join(*Relation) (*RelationUnit, error) func (*RelationUnit) Changes() <-chan RelationUnitsChange func (*RelationUnit) Depart() error func (*RelationUnit) Abscond() error ...in which Abscond seems to me to be the best available term for "depart quietly, abandoning rather than deleting the presence node, in the hope that departure will not be noticed by the other participants and the UA process will have time to restart and reoccupy the node without any other UA noticing". This will broadly mirror the behaviour of the UnitRelationState in python, but is IMO somewhat clearer (apart from anything else, departure is now explicit, rather than happening magically at process exit). YMMV; please let me know if it does, and how I could do it better :-). https://code.launchpad.net/~fwereade/juju-core/relation-units-watcher/+merge/115001 Requires: https://code.launchpad.net/~fwereade/juju-core/watch-presence-children/+merge/112375 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 17

Patch Set 2 : add relationUnitsWatcher #

Patch Set 3 : add relationUnitsWatcher #

Unified diffs Side-by-side diffs Delta from patch set Stats (+422 lines, -0 lines) Patch
A .lbox View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M state/relation_internal_test.go View 1 2 chunks +190 lines, -0 lines 0 comments Download
M state/watcher.go View 1 2 chunks +229 lines, -0 lines 0 comments Download

Messages

Total messages: 5
fwereade
Please take a look.
7 years, 7 months ago (2012-07-14 21:39:01 UTC) #1
niemeyer
Very happy with the overall thing. Thanks for persisting on this. https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go File state/relation_internal_test.go (right): ...
7 years, 7 months ago (2012-07-18 01:44:06 UTC) #2
fwereade
Please take a look. https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go File state/relation_internal_test.go (right): https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go#newcode109 state/relation_internal_test.go:109: _, err := s.st.zk.Create("/some-scope-path", "", ...
7 years, 7 months ago (2012-07-18 10:31:16 UTC) #3
niemeyer
LGTM! https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go File state/relation_internal_test.go (right): https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go#newcode109 state/relation_internal_test.go:109: _, err := s.st.zk.Create("/some-scope-path", "", 0, zkPermAll) On ...
7 years, 7 months ago (2012-07-18 19:08:23 UTC) #4
fwereade
7 years, 7 months ago (2012-07-19 07:44:24 UTC) #5
*** Submitted:

add relationUnitsWatcher

...which expresses changes to a relation from a given unit's perspective, in
terms that I believe will be helpful for the unit agent.

In particular, the main event type produced has Changed and Departed fields,
under the rationale that the UA will itself be keeping track of membership
as it does in the python version; and that therefore it is actually more
helpful to expect it to pay attention only to -changed and -departed events,
and to automatically run -joined hooks *before* running -changed hooks in
response to unrecognised units (rather than spearately tracking all 3 kinds
of event, and *also* inserting -changed events immediately after -joineds,
as is currently done in the python).

The intent is to keep this type internal, and use it only within a
(tentatively-named) RelationUnit type, which (1) exposes the
relationUnitsWatcher's Changes channel and (2) maintains a Pinger
signalling its own presence in the relation; I'm expecting it to
look something like:

func (*Unit) Join(*Relation) (*RelationUnit, error)

func (*RelationUnit) Changes() <-chan RelationUnitsChange
func (*RelationUnit) Depart() error
func (*RelationUnit) Abscond() error

...in which Abscond seems to me to be the best available term for "depart
quietly, abandoning rather than deleting the presence node, in the hope that
departure will not be noticed by the other participants and the UA process
will have time to restart and reoccupy the node without any other UA
noticing".

This will broadly mirror the behaviour of the UnitRelationState in python,
but is IMO somewhat clearer (apart from anything else, departure is now
explicit, rather than happening magically at process exit). YMMV; please
let me know if it does, and how I could do it better :-).

R=niemeyer
CC=
https://codereview.appspot.com/6405044

https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go
File state/relation_internal_test.go (right):

https://codereview.appspot.com/6405044/diff/1/state/relation_internal_test.go...
state/relation_internal_test.go:109: _, err :=
s.st.zk.Create("/some-scope-path", "", 0, zkPermAll)
On 2012/07/18 19:08:24, niemeyer wrote:
> On 2012/07/18 10:31:16, fwereade wrote:
> > How would you feel if I dropped these tests in the followup branch, which
does
> > test all this in terms of the public API?
> 
> Okay, if you say it's actually tested entirely through the public API in the
> follow up, I actually don't mind having this here for as long as it doesn't
> bother us. Can you please just add a comment to the top of the function.
> Something resembling:
> 
> // The logic exercised by this test is actually verified
> // through the public API as well in Test<name>.

Hmm, with the RelationHandler suggestion, I'm not sure precisely where it will
be tested any more. But I'll add the comment in the followup when I know where
it is :).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b