Merge lp:~themue/juju-core/024-cleanup-watcher into lp:~juju/juju-core/trunk

[Frank Mueller]

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.

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

[Frank Mueller (themue)]

Reviewers: mp+167748_code.launchpad.net,

Message:
Please take a look.

Description:
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.

https://code.launchpad.net/~themue/juju-core/024-cleanup-watcher/+merge/167748

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/10078043/

Affected files:
   A [revision details]
   M state/state_test.go
   M state/watcher.go

[William Reade (fwereade)]

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?

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

[Frank Mueller (themue)]

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.

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

[Frank Mueller (themue)]

Please take a look.

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

[Dimiter Naydenov (dimitern)]

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#newcode1362
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#newcode1390
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.

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

[William Reade (fwereade)]

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 t...

[William Reade (fwereade)]

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#newcode1357
state/state_test.go:1357: defer cw.Stop()
I think `defer stop(c, cw)` would be slightly more correct.

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

[lp:~themue/juju-core/024-cleanup-watcher]

1258. By Dimiter Naydenov on 2013-06-07

revert r1257 due to a failing test:

https://bugs.launchpad.net/juju-core/+bug/1188549

R=gz
CC=
https://codereview.appspot.com/10046047

[Frank Mueller (themue)]

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#newcode1357
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#newcode1390
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.

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

[Dimiter Naydenov (dimitern)]

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#newcode1363
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#newcode1383
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#newcode1415
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#newcode1420
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!

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

[lp:~themue/juju-core/024-cleanup-watcher]

1259. By Frank Mueller on 2013-06-07

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

[Frank Mueller (themue)]

*** 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#newcode1363
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#newcode1383
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#newcode1415
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#newcode1420
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.

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

=== modified file 'state/state_test.go'
--- state/state_test.go	2013-06-07 10:54:03 +0000
+++ state/state_test.go	2013-06-07 12:07:30 +0000
@@ -1236,3 +1236,93 @@
 	c.Assert(id, Equals, user.Name())
 	c.Assert(err, IsNil)
 }
+
+func (s *StateSuite) TestWatchCleanups(c *C) {
+	cw := s.State.WatchCleanups()
+	defer stop(c, cw)
+
+	assertNoChange := func() {
+		s.State.StartSync()
+		select {
+		case _, ok := <-cw.Changes():
+			c.Fatalf("unexpected change: %v", ok)
+		case <-time.After(50 * time.Millisecond):
+		}
+	}
+	assertChanges := func(count int) {
+		s.State.StartSync()
+		for i := 0; i < count; i++ {
+			select {
+			case _, ok := <-cw.Changes():
+				c.Assert(ok, Equals, true)
+			case <-time.After(500 * time.Millisecond):
+				c.Fatalf("timed out waiting for change")
+			}
+		}
+		assertNoChange()
+	}
+
+	// Check initial event.
+	assertChanges(1)
+
+	// Add relations doesn't emit events.
+	_, err := s.State.AddService("wordpress", s.AddTestingCharm(c, "wordpress"))
+	c.Assert(err, IsNil)
+	_, err = s.State.AddService("mysql", s.AddTestingCharm(c, "mysql"))
+	c.Assert(err, IsNil)
+	eps, err := s.State.InferEndpoints([]string{"wordpress", "mysql"})
+	c.Assert(err, IsNil)
+	relM, err := s.State.AddRelation(eps...)
+	c.Assert(err, IsNil)
+	assertNoChange()
+	_, err = s.State.AddService("varnish", s.AddTestingCharm(c, "varnish"))
+	c.Assert(err, IsNil)
+	eps, err = s.State.InferEndpoints([]string{"wordpress", "varnish"})
+	c.Assert(err, IsNil)
+	relV, err := s.State.AddRelation(eps...)
+	c.Assert(err, IsNil)
+	assertNoChange()
+
+	// Destroy relations and cleanup.
+	err = relM.Destroy()
+	c.Assert(err, IsNil)
+	assertChanges(1)
+	err = s.State.Cleanup()
+	c.Assert(err, IsNil)
+	assertChanges(1)
+	err = relV.Destroy()
+	c.Assert(err, IsNil)
+	assertChanges(1)
+	err = s.State.Cleanup()
+	c.Assert(err, IsNil)
+	assertChanges(1)
+
+	// A cleanup without need doesn't emit events.
+	err = s.State.Cleanup()
+	c.Assert(err, IsNil)
+	assertNoChange()
+
+	// Don't handle each event keeps them in the queue. Cleanups
+	// have a transaction per entry. So they create the according number
+	// of events. This behavior will change in a followup with a cleanup 
+	// redesign.
+	eps, err = s.State.InferEndpoints([]string{"wordpress", "mysql"})
+	c.Assert(err, IsNil)
+	relM, err = s.State.AddRelation(eps...)
+	c.Assert(err, IsNil)
+	assertNoChange()
+	eps, err = s.State.InferEndpoints([]string{"wordpress", "varnish"})
+	c.Assert(err, IsNil)
+	relV, err = s.State.AddRelation(eps...)
+	c.Assert(err, IsNil)
+	assertNoChange()
+	err = relM.Destroy()
+	c.Assert(err, IsNil)
+	err = relV.Destroy()
+	c.Assert(err, IsNil)
+	assertChanges(2)
+	err = s.State.Cleanup()
+	c.Assert(err, IsNil)
+	assertChanges(2)
+
+}

=== modified file 'state/watcher.go'
--- state/watcher.go	2013-06-07 10:54:03 +0000
+++ state/watcher.go	2013-06-07 12:07:30 +0000
@@ -1231,3 +1231,55 @@
 	}
 	panic("unreachable")
 }
+
+// CleanupWatcher notifies changes of the cleanup documents 
+// signalling the need for a cleanup.
+type CleanupWatcher struct {
+	commonWatcher
+	out chan struct{}
+}
+
+// WatchCleanups returns a CleanupWatcher that notifies when documents
+// that where marked for removal exist.
+func (st *State) WatchCleanups() *CleanupWatcher {
+	return newCleanupWatcher(st)
+}
+
+func newCleanupWatcher(st *State) *CleanupWatcher {
+	w := &CleanupWatcher{
+		commonWatcher: commonWatcher{st: st},
+		out:           make(chan struct{}),
+	}
+	go func() {
+		defer w.tomb.Done()
+		defer close(w.out)
+		w.tomb.Kill(w.loop())
+	}()
+	return w
+}
+
+// Changes returns the event channel for w.
+func (w *CleanupWatcher) Changes() <-chan struct{} {
+	return w.out
+}
+
+func (w *CleanupWatcher) loop() (err error) {
+	in := make(chan watcher.Change)
+
+	w.st.watcher.WatchCollection(w.st.cleanups.Name, in)
+	defer w.st.watcher.UnwatchCollection(w.st.cleanups.Name, in)
+
+	// Initial event.
+	w.out <- struct{}{}
+
+	for {
+		select {
+		case <-w.tomb.Dying():
+			return tomb.ErrDying
+		case <-in:
+			// Simply emit event for each change.
+			w.out <- struct{}{}
+		}
+	}
+	panic("unreachable")
+}