Code review comment for lp:~wallyworld/juju-core/provisioner-retry
Revision history for this message
| Ian Booth (wallyworld) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
7020100
(Get the code!)
Please take a look.
https:/
File state/apiserver
https:/
state/apiserver
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:/
state/apiserver
minutes.
On 2014/03/26 07:22:42, jameinel wrote:
> Your comment here doesn't match the actual ErrorRetryWaitD
just
> reference the variable instead of an explicit time ?
Doh, will fix.
https:/
state/apiserver
<-time.
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:/
File worker/
https:/
worker/
s.PatchValue(
50*time.
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:/
worker/
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:/