Issue 10078043: state: added CleanupWatcher (Closed)
Descriptionstate: added CleanupWatcher
The CleanupWatcher signals the demand for the running of
state.Cleanup(). It is the first in a row of CLs to add
an according worker.
https://code.launchpad.net/~themue/juju-core/024-cleanup-watcher/+merge/167748
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 5
Patch Set 2 : state: added CleanupWatcher #
Total comments: 14
Patch Set 3 : state: added CleanupWatcher #
Total comments: 13
Patch Set 4 : state: added CleanupWatcher #
MessagesTotal messages: 10
Please take a look.
Sign in to reply to this message.
Sorry, NOT LGTM. This does not do what we agreed (notify on any change to the cleanups collection), does stuff we don't want (notifies even if there are no changes to the cleanups collection) is not self-consistent (an empty collection sometimes generates an event, and sometimes does not)... and is *not a watcher*, either in form or in function. 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#newcode1244 state/watcher.go:1244: // After a unit is found to be Dead, no further event will include it. Fix these please :). https://codereview.appspot.com/10078043/diff/1/state/watcher.go#newcode1283 state/watcher.go:1283: next := time.After(watcher.Period) Why is this not a... watcher? We discussed this yesterday: there's no call whatsoever to make this periodic. We can just use the usual, conventional mechanism of watching the st.cleanups collection, but with the additional simplification that I'm happy just sending out a notification whenever there's any change whatsoever to any document in there. I really tried to be as explicit as possible; what do you think is the source of the communication problems here?
Sign in to reply to this message.
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#newcode1244 state/watcher.go:1244: // After a unit is found to be Dead, no further event will include it. On 2013/06/06 16:26:44, fwereade wrote: > Fix these please :). Ouch, yes! 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:26:44, fwereade wrote: > Why is this not a... watcher? We discussed this yesterday: there's no call > whatsoever to make this periodic. We can just use the usual, conventional > mechanism of watching the st.cleanups collection, but with the additional > simplification that I'm happy just sending out a notification whenever there's > any change whatsoever to any document in there. 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. 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? > 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.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
A few comments. https://codereview.appspot.com/10078043/diff/6001/state/state.go File state/state.go (right): https://codereview.appspot.com/10078043/diff/6001/state/state.go#newcode1079 state/state.go:1079: TxnRevno int64 `bson:"txn-revno"` You don't need to define this, it's automatically available. 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#newcode... state/state_test.go:1362: case _, ok := <-cw.Changes(): Please add c.Assert(ok, Equals, true), otherwise this is not a change notification. https://codereview.appspot.com/10078043/diff/6001/state/state_test.go#newcode... state/state_test.go:1390: _, err = s.State.AddService("varnish", s.AddTestingCharm(c, "varnish")) do you really need to do this twice? surely only one relation followed by destroy should be sufficient, but ymmv 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#newcode1256 state/watcher.go:1256: in chan watcher.Change in chan doesn't need to be a field of the watcher, you can create it locally inside loop() https://codereview.appspot.com/10078043/diff/6001/state/watcher.go#newcode1288 state/watcher.go:1288: out := w.out is there always an initial event that makes this line necessary? istm this will always cause a notification to be sent, even when there's nothing coming on the in chan.
Sign in to reply to this message.
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#newcode... 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#newcode... 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.
Sign in to reply to this message.
just another comment on the tests 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#newcode... state/state_test.go:1357: defer cw.Stop() I think `defer stop(c, cw)` would be slightly more correct.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/10078043/diff/6001/state/state.go File state/state.go (right): https://codereview.appspot.com/10078043/diff/6001/state/state.go#newcode1079 state/state.go:1079: TxnRevno int64 `bson:"txn-revno"` On 2013/06/07 09:15:37, dimitern wrote: > You don't need to define this, it's automatically available. Done. 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#newcode... state/state_test.go:1357: defer cw.Stop() On 2013/06/07 09:40:10, fwereade wrote: > I think `defer stop(c, cw)` would be slightly more correct. Done. https://codereview.appspot.com/10078043/diff/6001/state/state_test.go#newcode... state/state_test.go:1390: _, err = s.State.AddService("varnish", s.AddTestingCharm(c, "varnish")) On 2013/06/07 09:35:44, fwereade wrote: > 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. Added some more and detected, that Cleanup() removes each document in an own transaction which leads to more events. Will change this later in a followup. Tests currently handle it. 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#newcode1256 state/watcher.go:1256: in chan watcher.Change On 2013/06/07 09:15:37, dimitern wrote: > in chan doesn't need to be a field of the watcher, you can create it locally > inside loop() Done. https://codereview.appspot.com/10078043/diff/6001/state/watcher.go#newcode1288 state/watcher.go:1288: out := w.out On 2013/06/07 09:15:37, dimitern wrote: > is there always an initial event that makes this line necessary? istm this will > always cause a notification to be sent, even when there's nothing coming on the > in chan. It only sent the initial event, afterwards out has been nil and so no event has been sent. When theres a signal via in out is changed to the out channel again, sents one event and is set to nil again. But I changed it to a simpler way.
Sign in to reply to this message.
LGTM with a few comments changed. https://codereview.appspot.com/10078043/diff/14002/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/10078043/diff/14002/state/state_test.go#newcod... state/state_test.go:1363: c.Fatalf("unexpected change: %v", ok) please change this to say "unexpected change; ok: %v" https://codereview.appspot.com/10078043/diff/14002/state/state_test.go#newcod... state/state_test.go:1383: // Add relations doesn't emit events. // Adding relations doesn't emit events. https://codereview.appspot.com/10078043/diff/14002/state/state_test.go#newcod... state/state_test.go:1415: // A cleanup without need doesn't emit events. // Verify that Cleanup() doesn't emit unnecessary events. https://codereview.appspot.com/10078043/diff/14002/state/state_test.go#newcod... state/state_test.go:1420: // Don't handle each event keeps them in the queue. Cleanups // Not calling Cleanup() queues up the changes, so that // multiple changes will be emitted the next time Cleanup() // is called. This behavior will change in a follow-up. https://codereview.appspot.com/10078043/diff/14002/state/watcher.go File state/watcher.go (right): https://codereview.appspot.com/10078043/diff/14002/state/watcher.go#newcode1251 state/watcher.go:1251: // CleanupWatcher notifies changes of the cleanup documents // CleanupWatcher notifies of changes in the cleanups collection. ? https://codereview.appspot.com/10078043/diff/14002/state/watcher.go#newcode1258 state/watcher.go:1258: // WatchCleanups returns a CleanupWatcher that notifies when documents // WatchCleanups starts and returns a CleanupWatcher. https://codereview.appspot.com/10078043/diff/14002/state/watcher.go#newcode1289 state/watcher.go:1289: w.out <- struct{}{} nice!
Sign in to reply to this message.
*** Submitted: state: added CleanupWatcher The CleanupWatcher signals the demand for the running of state.Cleanup(). It is the first in a row of CLs to add an according worker. R=fwereade, dimitern CC= https://codereview.appspot.com/10078043 https://codereview.appspot.com/10078043/diff/14002/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/10078043/diff/14002/state/state_test.go#newcod... state/state_test.go:1363: c.Fatalf("unexpected change: %v", ok) On 2013/06/07 12:24:50, dimitern wrote: > please change this to say "unexpected change; ok: %v" Done. https://codereview.appspot.com/10078043/diff/14002/state/state_test.go#newcod... state/state_test.go:1383: // Add relations doesn't emit events. On 2013/06/07 12:24:50, dimitern wrote: > // Adding relations doesn't emit events. Done. https://codereview.appspot.com/10078043/diff/14002/state/state_test.go#newcod... state/state_test.go:1415: // A cleanup without need doesn't emit events. On 2013/06/07 12:24:50, dimitern wrote: > // Verify that Cleanup() doesn't emit unnecessary events. Done. https://codereview.appspot.com/10078043/diff/14002/state/state_test.go#newcod... state/state_test.go:1420: // Don't handle each event keeps them in the queue. Cleanups On 2013/06/07 12:24:50, dimitern wrote: > // Not calling Cleanup() queues up the changes, so that > // multiple changes will be emitted the next time Cleanup() > // is called. This behavior will change in a follow-up. That's not correct, the second part is related to the internal behavior of Cleanup() with individual transactions per deletion. Will separate this better. https://codereview.appspot.com/10078043/diff/14002/state/watcher.go File state/watcher.go (right): https://codereview.appspot.com/10078043/diff/14002/state/watcher.go#newcode1251 state/watcher.go:1251: // CleanupWatcher notifies changes of the cleanup documents On 2013/06/07 12:24:50, dimitern wrote: > // CleanupWatcher notifies of changes in the cleanups collection. > ? Done. https://codereview.appspot.com/10078043/diff/14002/state/watcher.go#newcode1258 state/watcher.go:1258: // WatchCleanups returns a CleanupWatcher that notifies when documents On 2013/06/07 12:24:50, dimitern wrote: > // WatchCleanups starts and returns a CleanupWatcher. Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
