Please take a look.


https://codereview.appspot.com/80340043/diff/20001/state/apiserver/provisioner/machineerror.go
File state/apiserver/provisioner/machineerror.go (right):

https://codereview.appspot.com/80340043/diff/20001/state/apiserver/provisioner/machineerror.go#newcode20
state/apiserver/provisioner/machineerror.go:20:
On 2014/03/26 07:22:42, jameinel wrote:
> I really wish we could inherit from a common NotifyWatcher
implementation and
> then just implement the "loop()" in our custom code.

> I think we could actually do that with a NotifyWatchLooper or some
such that did
> all of this work and just took an

> type interface Looper {
>    Loop(out chan struct{}) error
> }

> I've done some bits like that in the Workers, where I implemented
NotifyWorker
> that takes a Handler that triggers when the changes are actually
interesting.

> Anyway, not something you have to do here, because our pattern in code
has
> definitely been to just copy & paste the boilerplate.

Yeah, I agree. Tim and I have looked into this before and because Go
doesn't have things like virtual methods or generics it is really hard
to avoid all the boiler plate. I was in a rush for this branch to try
and make the 1.18 deadline so I didn't look into it oo much for this
branch.

https://codereview.appspot.com/80340043/diff/20001/state/apiserver/provisioner/machineerror.go#newcode66
state/apiserver/provisioner/machineerror.go:66: // triggering every 2
minutes.
On 2014/03/26 07:22:42, jameinel wrote:
> Your comment here doesn't match the actual ErrorRetryWaitDelay. Maybe
just
> reference the variable instead of an explicit time ?

Doh, will fix.

https://codereview.appspot.com/80340043/diff/20001/state/apiserver/provisioner/machineerror.go#newcode73
state/apiserver/provisioner/machineerror.go:73: case
<-time.After(ErrorRetryWaitDelay):
On 2014/03/26 07:22:42, jameinel wrote:
> But all of this boiler plate exists *only* for this line. (when this
channel
> triggers, trigger an output).

For now yes. But this is just the start. There's a lot more business
logic to be added. The time based trigger is just to get something
working.

https://codereview.appspot.com/80340043/diff/20001/worker/provisioner/provisioner_test.go
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/80340043/diff/20001/worker/provisioner/provisioner_test.go#newcode773
worker/provisioner/provisioner_test.go:773:
s.PatchValue(&apiserverprovisioner.ErrorRetryWaitDelay,
50*time.Millisecond)
On 2014/03/26 07:22:42, jameinel wrote:
> Can we use testing.ShortWait here instead of another hard-coded time?

Sure. Originally it wasn't ShortWait but it is now so I'll change it.

https://codereview.appspot.com/80340043/diff/20001/worker/provisioner/provisioner_test.go#newcode804
worker/provisioner/provisioner_test.go:804: // Machine 4 is never
provisioned.
On 2014/03/26 07:22:42, jameinel wrote:
> I don't understand why Machine 4 is listed as never provisioned, when
the
> StartInstance calls claim that it will be provisioned after 2 retries.
> Is this sensitive to the timeout times? (we assume we poll at exactly
50ms, and
> we get 2 polls in the 100ms wait above?)


Only machine 3 is provisioned after 2 retries. The stupid comment is
wrong - sorry :-( I'll fix it. The mockBroker never provisions machine
4.

> Anything that requires the timing to get right tends to fail on the
bot.

> It may be that checkStartInstance properly retries until success, or
something
> else.

> Is there even a reason why we need to have ErrorRetryWaitDelay not
something
> like even 5ms?

There's no new timing dependencies in the test. There's an existing 2
max second wait that checkProvisioned uses. I set the 100ms loop delay
before the retry flag is set merely to ensure that at least one
provisioner retry has occurred before setting the flag. I can change
50ms to ShortWait

https://codereview.appspot.com/80340043/