Code review comment for lp:~dave-cheney/pyjuju/go-provisioning-strawman
Revision history for this message
| Gustavo Niemeyer (niemeyer) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
https:/
File cmd/jujud/
https:/
cmd/jujud/
On 2012/06/01 01:58:27, dfc wrote:
> > What happens if the connection is broken? We'll need to reestablish
it and
> > restart from the beginning, since all the assumptions are now wrong.
Maybe
> this
> > has to be moved into the outer loop or something. Will leave details
with you
> at
> > this point.
> I looked into this more this morning and it looks like if the
underlying
> connection to ZK is interrupted then the c client reconnects behind
the scenes.
> I tested this by running the PA, then shutting down zk, then starting
it again a
> minute later. None of the watchers exited.
> I'm going to keep playing with this, but at this point, if gozk never
reports an
> error when it cant talk to the zk server, I can't think of a way to
detect
> connection failure.
We've covered this on juju-dev@. A TODO would be good here as well.
https:/
File cmd/jujud/
https:/
cmd/jujud/
will receive the new *ConfigNode
As we discussed, that logic can be simplified as we can bubble up onto
the top from any hiccups, rather than retrying in place. I'm happy both
for this branch to be changed to cover that, or for it to be integrated
with these and then refactored. I'll comment in-place and let the
decision with you.
https:/
cmd/jujud/
// when a change in the environment configuration is detected.
https:/
cmd/jujud/
invalidates the current environWatcher.
s/and invalidates//
https:/
cmd/jujud/
reported error on Stop: %v", err)
This is Printf rather than Debugf, so we need to be a bit nicer to the
user (no method and field names). Something like this might do:
"environment configuration watcher: %v"
https:/
cmd/jujud/
receive the new *ConfigNode when a
Needs updating.
https:/
cmd/jujud/
reported error on Stop: %v", err)
"machines watcher: %v"
https:/
cmd/jujud/
state.IsValid() here to exit cleanly if
IsConnected?
https:/
cmd/jujud/
This should return an error.. it won't help to continue here. The state
will be broken.
https:/
cmd/jujud/
environment from supplied configuration: %v", err)
"provisioner loaded invalid environment configuration: %v"
https:/
cmd/jujud/
environment configured")
"provisioner loaded new environment configuration"
We spent a good while in London fiddling with messages and trying to
imagine how the actual error and log messages look like with prefixes,
and the conclusion we got to is that it doesn't really improve much the
understanding, and that we've been terribly inconsistent so far. The
agreement we're trying to get to (which still accepts input) is being a
bit more clear in the messages themselves.
https:/
cmd/jujud/
state.IsValid() here to exit cleanly if
IsConnected?
https:/
cmd/jujud/
closed")
"environment configuration watcher was closed"
https:/
cmd/jujud/
configuration received, but was not valid: %v", err)
"provisioner loaded invalid environment configuration: %v"
https:/
cmd/jujud/
configuartion applied")
"provisioner loaded new environment configuration"
https:/