HookQueue now looks more like a watcher

HookQueue now looks more like a watcher



https://code.launchpad.net/~fwereade/juju-core/hook-queue-watcher-style/+merge/116684

(do not edit description out of merge proposal)

[fwereade, 2012-07-25 15:37:35]

Please take a look.

[niemeyer, 2012-07-27 11:09:58]

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go
File worker/uniter/hookqueue.go (right):

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:31: // from the supplied RelationWatcher.
// NewHookQueue returns a new HookQueue that aggregates the values obtained
// from the w watcher and sends into out the details about hooks that must
// be executed in the unit.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:43: // and sends them on to a client.
// HookQueue aggregates values obtained from a relation settings watcher
// and sends out details about hooks that must be executed in the unit.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:71: unit string
Some spacing in the fields below, as done in HookQueue above, would be helpful.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:79: // true for the remaining lifetime of the
unitInfo.
// joined is set to true when a "joined" is popped for this unit.
joined bool

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:94: for {
This loop is great.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:95: if q.head == nil && q.joined == "" {
if q.empty() {

The comment below may be dropped then, as the code becomes obvious.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:135: info, found := q.info[unit]
Here is a version that I think does the same thing as the logic below but is
significantly simpler:

if !found {
    info = &unitInfo{unit: unit}
    q.add(unit, "joined")
} else if settings.Version != info.Version {
    q.add(unit, "changed")
} else {
    q.remove(unit)
}
info.version = settings.Version
info.settings = settings.Settings

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:174: if q.joined != "" {
After reading the logic a few times, I think we should rename this to
changedPending instead, so that we're recording what we *want* to happen, rather
than what we did in the past. All of the code around the use of this field is
related to the "changed" event, not to joined (see below).

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:197: func (q *HookQueue) setNext() {
Rather than having that method being called on any changes to set a field, we
can drop the field, drop all the call sites, call the method as "next" and call
it at the single call site where we use q.next.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:199: if q.joined != "" {
if q.empty() {
    panic("queue is empty")
}
if q.joined != "" {
    unit = q.joined
    hook = "changed"
} else {
    unit = q.head.unit
    hook = q.head.hook
}

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:229: func (q *HookQueue) add(unit, hook string) {
s/add/enqueue/; this is specially relevant on the remove side, because
HookQueue.remove(unit) doesn't really remove the unit from the HookQueue for
real.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:250: func (q *HookQueue) remove(unit string) {
s/remove/dequeue/

[fwereade, 2012-07-27 14:35:03]

*** Submitted:

HookQueue now looks more like a watcher

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

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go
File worker/uniter/hookqueue.go (right):

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:31: // from the supplied RelationWatcher.
On 2012/07/27 11:09:58, niemeyer wrote:
> // NewHookQueue returns a new HookQueue that aggregates the values obtained
> // from the w watcher and sends into out the details about hooks that must
> // be executed in the unit.

Done.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:43: // and sends them on to a client.
On 2012/07/27 11:09:58, niemeyer wrote:
> // HookQueue aggregates values obtained from a relation settings watcher
> // and sends out details about hooks that must be executed in the unit.

Done.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:71: unit string
On 2012/07/27 11:09:58, niemeyer wrote:
> Some spacing in the fields below, as done in HookQueue above, would be
helpful.

Done.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:79: // true for the remaining lifetime of the
unitInfo.
On 2012/07/27 11:09:58, niemeyer wrote:
> // joined is set to true when a "joined" is popped for this unit.
> joined bool

Done.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:94: for {
On 2012/07/27 11:09:58, niemeyer wrote:
> This loop is great.

:D

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:95: if q.head == nil && q.joined == "" {
On 2012/07/27 11:09:58, niemeyer wrote:
> if q.empty() {
> 
> The comment below may be dropped then, as the code becomes obvious.

Done.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:135: info, found := q.info[unit]
On 2012/07/27 11:09:58, niemeyer wrote:
> Here is a version that I think does the same thing as the logic below but is
> significantly simpler:
> 
> if !found {
>     info = &unitInfo{unit: unit}
>     q.add(unit, "joined")
> } else if settings.Version != info.Version {
>     q.add(unit, "changed")
> } else {
>     q.remove(unit)
> }
> info.version = settings.Version
> info.settings = settings.Settings

Not quite; but niemeyer figured it out live, and it's awesome.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:174: if q.joined != "" {
On 2012/07/27 11:09:58, niemeyer wrote:
> After reading the logic a few times, I think we should rename this to
> changedPending instead, so that we're recording what we *want* to happen,
rather
> than what we did in the past. All of the code around the use of this field is
> related to the "changed" event, not to joined (see below).

Done.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:197: func (q *HookQueue) setNext() {
On 2012/07/27 11:09:58, niemeyer wrote:
> Rather than having that method being called on any changes to set a field, we
> can drop the field, drop all the call sites, call the method as "next" and
call
> it at the single call site where we use q.next.

Done.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:199: if q.joined != "" {
On 2012/07/27 11:09:58, niemeyer wrote:
> if q.empty() {
>     panic("queue is empty")
> }
> if q.joined != "" {
>     unit = q.joined
>     hook = "changed"
> } else {
>     unit = q.head.unit
>     hook = q.head.hook
> }

Done.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:229: func (q *HookQueue) add(unit, hook string) {
On 2012/07/27 11:09:58, niemeyer wrote:
> s/add/enqueue/; this is specially relevant on the remove side, because
> HookQueue.remove(unit) doesn't really remove the unit from the HookQueue for
> real.

We'll go with queue/unqueue as discussed live. Done.

https://codereview.appspot.com/6453043/diff/1/worker/uniter/hookqueue.go#newc...
worker/uniter/hookqueue.go:250: func (q *HookQueue) remove(unit string) {
On 2012/07/27 11:09:58, niemeyer wrote:
> s/remove/dequeue/

See above.