state/watcher: remove Sync

state/watcher: remove Sync

The intended difference between Sync and StartSync is that Sync makes
sure that all events have been sent on the out channel. This isn't
something that clients should be sensitive to.

But clients *can* tell when it starts to watch something if the watcher
is acting on old information or not (it affects the initial event)
and the old implementation of StartSync made that very hard to rely on.

So this CL removes Sync entirely as being misguided, but makes StartSync
itself more deterministic by ensuring that when the watcher loop receives
a sync request, it will not deal with any more events until it has
synchronised. Note that the new test in watcher_test.go consistently
failed with the old implementation.

Note that the state/presence package contains very similar logic
that should be changed in the same way.  Until that is fixed, we use
Presence.Sync inside State.StartSync.

In the next CL after that one, I plan to rename StartSync to Sync
throughout, as there's no need for the "Start" qualification if there
is only one form.

https://code.launchpad.net/~rogpeppe/juju-core/372-startsync/+merge/181230

(do not edit description out of merge proposal)

[rog, 2013-08-21 09:32:49]

Please take a look.

[mue, 2013-08-21 09:55:16]

On 2013/08/21 09:32:49, rog wrote:
> Please take a look.

LGTM, I only have a small problem with the name StartSync(). It sounds to me
like the starting of an asynchronous process instead of a synchronous syncing.
But as far as I understood the syncing has been done when the method is left. So
Sync() IMHO would be the better name.

[jameinel, 2013-08-21 13:00:48]

LGTM

I'm a little concerned that we didn't add a test for the LifeCycleWatcher
change, but moving the Sync code is fine with me.

https://codereview.appspot.com/13089045/diff/5001/state/watcher.go
File state/watcher.go (left):

https://codereview.appspot.com/13089045/diff/5001/state/watcher.go#oldcode66
state/watcher.go:66: var count int
I don't think you have to move this to another CL, but I do wish that you would
have at least mentioned it in the cover letter.

LifecycleWatcher coalescing its changes is a good thing, and a pretty notable
change to have it silently added.

I might have missed it, but I don't actually see a test change that tracks that
LifecycleWatcher is aggregating its results. Is there one that I just missed?

https://codereview.appspot.com/13089045/diff/5001/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/13089045/diff/5001/state/watcher/watcher.go#ne...
state/watcher/watcher.go:211: next := time.After(Period)
I actually really like this change. Not because it syncs immediately, but
because I feel it is much easier to reason about what is being done and when.

[jameinel, 2013-08-21 13:15:36]

So I was wrong about what class was getting collection. Just a suggestion that
you mention "And I updated CleanupWatcher to call collect() because otherwise
the tests fail under the new structure".

Still LGTM, though. Sorry about the red herring on LifecycleWatcher.

https://codereview.appspot.com/13089045/diff/5001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/13089045/diff/5001/state/watcher.go#newcode1274
state/watcher.go:1274: // Simply emit event for each change.
Given we are collecting here, is this a correct comment now?

[rog, 2013-08-21 15:18:12]

Please take a look.

https://codereview.appspot.com/13089045/diff/5001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/13089045/diff/5001/state/watcher.go#newcode1274
state/watcher.go:1274: // Simply emit event for each change.
On 2013/08/21 13:15:36, jameinel wrote:
> Given we are collecting here, is this a correct comment now?

good point. done.