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

Issue 6402048: add RelationUnit type

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 2 months ago by fwereade
Modified:
8 years, 2 months ago
Reviewers:
mp+115030, dfc, niemeyer
Visibility:
Public.

Description

add RelationUnit type A RelationUnit maintains a unit's presence in a relation, and watches changes to other RelationUnits running in other unit agents. https://code.launchpad.net/~fwereade/juju-core/relation-unit/+merge/115030 Requires: https://code.launchpad.net/~fwereade/juju-core/relation-units-watcher/+merge/115001 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 2

Patch Set 2 : add RelationUnit type #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+651 lines, -0 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M state/presence/presence.go View 1 chunk +6 lines, -0 lines 0 comments Download
M state/relation.go View 2 chunks +95 lines, -0 lines 0 comments Download
M state/relation_test.go View 1 2 chunks +501 lines, -0 lines 0 comments Download
M state/unit.go View 1 chunk +47 lines, -0 lines 6 comments Download

Messages

Total messages: 5
fwereade
Please take a look.
8 years, 2 months ago (2012-07-15 15:28:50 UTC) #1
dfc
https://codereview.appspot.com/6402048/diff/1/state/confignode.go File state/confignode.go (right): https://codereview.appspot.com/6402048/diff/1/state/confignode.go#newcode177 state/confignode.go:177: // lp:~fwereade/juju-core/config-node-bug; drop this note Please address the TODO ...
8 years, 2 months ago (2012-07-16 10:03:12 UTC) #2
fwereade
Please take a look.
8 years, 2 months ago (2012-07-16 12:22:16 UTC) #3
niemeyer
This is looking *great*. Seems very close to one of the trickiest pieces in the ...
8 years, 2 months ago (2012-07-18 21:54:40 UTC) #4
fwereade
8 years, 2 months ago (2012-07-19 15:39:26 UTC) #5
I'm going to reject this CL and build the suggested RelationHandler methods into
Relation itself in new branches.

https://codereview.appspot.com/6402048/diff/6001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6402048/diff/6001/state/unit.go#newcode537
state/unit.go:537: func (u *Unit) AgentJoin(r *Relation) (*RelationUnit, error)
{
On 2012/07/18 21:54:40, niemeyer wrote:
> Some thinking should probably be put into the error reporting in these
> functions. Right now it returns everything as-is, and it might get confusing
in
> some cases to tell what's actually failing.

Good point, thanks.

https://codereview.appspot.com/6402048/diff/6001/state/unit.go#newcode551
state/unit.go:551: pkey, err := topo.UnitPrincipalKey(u.key)
On 2012/07/18 21:54:40, niemeyer wrote:
> We used to have the unit's principal key in u.principalKey, and we have it at
> hand when we create the unit. It's extremely cheap and simplifies this logic,
so
> I suggest putting it back (and keeping isPrincipal as-is) so we can just use
it
> here and drop the topology-reading entirely.

Addressed in https://codereview.appspot.com/6421049

https://codereview.appspot.com/6402048/diff/6001/state/unit.go#newcode565
state/unit.go:565: // will know it's time to add it.
On 2012/07/18 21:54:40, niemeyer wrote:
> I suggest cutting down the comment to:
> 
> // The unit agent should have set the private address by now.
> 
> The rest is timing-dependent information that will become out-of-date very
soon,
> and doesn't add to the above.

Cheers.
Sign in to reply to this message.

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