Code review comment for lp:~themue/juju-core/024-cleanup-watcher
Revision history for this message
| William Reade (fwereade) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
0e1f616
(Get the code!)
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:/
File state/watcher.go (right):
https:/
state/watcher.
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-
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:/
File state/state_test.go (right):
https:/
state/state_
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:/
state/state_
s.AddTestingCha
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:/
File state/watcher.go (right):
https:/
state/watcher.
w.st.watcher.
Thank you for fixing this.
https:/