LGTM with dimitern's comments addressed and some better tests for the
watcher. Sorry for the partially-outdated essay in the previous change
set.


https://codereview.appspot.com/10078043/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10078043/diff/1/state/watcher.go#newcode1283
state/watcher.go:1283: next := time.After(watcher.Period)
On 2013/06/06 16:54:17, mue wrote:
> The usual mechanism relies on entities with TxnRevno. I wanted to use
it first,
> but then I've seen that the cleanupDoc doesn't uses it. Es explicit as
most of
> our code is I thought that this decision has not been by accident. So
I
> implemented a polling with the same timeframe the standard watcher
uses to
> recognize changes.

You don't need TxnRevno at all. The txn-revno field on which we depend
is written by mgo/txn regardless, and you don't need to use the field in
the state package *except* when starting a single-entity watch on a
document; and relatively few of our document types actually require that
functionality.

The watching code is all present and correct and already set up for this
scenario; there is *no* justification possible for *polling* data that
changes approximately 0 times per month. Just watch the cleanups
collection and send an empty notification *every* time *anything*
happens to the collection (in addition to the conventionally-guaranteed
initial event).

> I surely can change it to the standard watcher adding the TxnRevno. I
only see
> that it leads to the sending of an event on any change. This would
later lead to
> events during the cleanup and so also lead to even more Cleanup()
calls. I've
> tried to avoid it. How would you avoid those events during/after a
Cleanup()
> run?

Events during the cleanups will be coalesced by a sane implementation,
and thus lead to *one* extra cleanup call. Some months will go by
without this consequence ever coming to pass, so I think we can live
with it... as I thought we agreed the other day. If we ever decide we
can't afford that cost, we can write a more sophisticated watcher.

> > I really tried to be as explicit as possible; what do you think is
the source
> of
> > the communication problems here?

> That's a different topic and we tomorrow should take some time to have
a talk
> via Hangout.

I'm on holiday today.

https://codereview.appspot.com/10078043/diff/6001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/10078043/diff/6001/state/state_test.go#newcode1362
state/state_test.go:1362: case _, ok := <-cw.Changes():
On 2013/06/07 09:15:37, dimitern wrote:
> Please add c.Assert(ok, Equals, true), otherwise this is not a change
> notification.

Meh, -1, the fatal message includes the value, it's never getting lost.
Don't see what'd be improved by a distinct assert.

https://codereview.appspot.com/10078043/diff/6001/state/state_test.go#newcode1390
state/state_test.go:1390: _, err = s.State.AddService("varnish",
s.AddTestingCharm(c, "varnish"))
On 2013/06/07 09:15:37, dimitern wrote:
> do you really need to do this twice? surely only one relation followed
by
> destroy should be sufficient, but ymmv

It'd support more sophisticated testing -- for event coalescing, effect
of Cleanup calls, etc -- quite nicely. Would be nice to see tests for
these cases.

https://codereview.appspot.com/10078043/diff/6001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10078043/diff/6001/state/watcher.go#newcode1286
state/watcher.go:1286: defer
w.st.watcher.UnwatchCollection(w.st.cleanups.Name, w.in)
Thank you for fixing this.

https://codereview.appspot.com/10078043/