state: Second approach to improve the error handling.

state: Second approach to improve the error handling.

This second approach returns error generated with fmt.Errorf()
and containing high-level informations like names. In case of
an internal error this is appended or, in case of some topology
errors, directly returned. This depends on the returned error
of the topology method.

https://code.launchpad.net/~themue/juju-core/go-state-second-error-improvement/+merge/109860

(do not edit description out of merge proposal)

[TheMue, 2012-06-12 14:23:36]

Please take a look.

[niemeyer, 2012-06-12 15:07:00]

https://codereview.appspot.com/6296064/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6296064/diff/1/state/state.go#newcode64
state/state.go:64: return fmt.Errorf("can't remove machine %d: %v", id, err)
This is misleading. The machine has been removed from the topology for good.
This is just cleaning up the tree. The message may be worded like:

"can't clean up data for machine %d: %v"

https://codereview.appspot.com/6296064/diff/1/state/state.go#newcode171
state/state.go:171: return nil, fmt.Errorf("can't add service %q: %v", name,
err)
The amount of repetition in this file is hurting. A mechanism like this might be
useful:

defer errorContext(&err, "can't add service %q: %v", name)

[rog, 2012-06-12 15:23:06]

https://codereview.appspot.com/6296064/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6296064/diff/1/state/state.go#newcode32
state/state.go:32: return nil, fmt.Errorf("can't add a new machine: %v", err)
I'm not sure this style is right.

When we call AddMachine we know we're adding a new machine, so the fact that the
error says "can't add new machine" adds no information. I think that the error
should describe why AddMachine failed, which is not "because it can't add a new
machine" (we know that from the fact that it's AddMachine returning an error)
but whatever it was doing when it failed. For example, "cannot create new
machine node" in this case, or "cannot add machine to topology" in the case
below.

In some cases, we might decide that the error returned from the sub-path is
appropriate for return from the function (for instance, we might consider that
`zookeeper: create "/machines-":  connection loss` makes sense to return here,
in which case we can return the error unannotated.

Does that make sense?

[niemeyer, 2012-06-12 19:52:46]

https://codereview.appspot.com/6296064/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6296064/diff/1/state/state.go#newcode32
state/state.go:32: return nil, fmt.Errorf("can't add a new machine: %v", err)
On 2012/06/12 15:23:06, rog wrote:
> I'm not sure this style is right.
> 
> When we call AddMachine we know we're adding a new machine, so the fact that
the
> error says "can't add new machine" adds no information. I think that the error
> should describe why AddMachine failed, which is not "because it can't add a
new
> machine" (we know that from the fact that it's AddMachine returning an error)
> but whatever it was doing when it failed. For example, "cannot create new
> machine node"

Can't create new node is what ZooKeeper is supposed to tell, not AddMachine.
That said, following your suggestion Create can't say that it can't create a new
node either, because when we call Create we know that we're creating a node.

But then you point out that it's actually fine for Create to return an error
pointing out that creation failed, and that AddMachine doesn't have to say so in
that case. Well.. if that's the idea, AddMachine may well decorate the error
too, so that its callers don't have to tell that it was adding a machine that
failed.

> Does that make sense?

Not entirely (to me, at least).  The suggestion seems inconsistent.

[TheMue, 2012-06-13 11:30:28]

Please take a look.

[rog, 2012-06-13 12:12:37]

> > I'm not sure this style is right.
> > 
> > When we call AddMachine we know we're adding a new machine, so the fact that
> the
> > error says "can't add new machine" adds no information. I think that the
error
> > should describe why AddMachine failed, which is not "because it can't add a
> new
> > machine" (we know that from the fact that it's AddMachine returning an
error)
> > but whatever it was doing when it failed. For example, "cannot create new
> > machine node"
> 
> Can't create new node is what ZooKeeper is supposed to tell, not AddMachine.

That's close to the heart of this issue, I think.

I think the reason that all the zookeeper errors are annotated with
the path and the operation is because we've made a judgement that those
errors are likely to make sense when returned from contexts other than
zookeeper methods (the path, in particular, disambiguates the message
in many contexts), and because the cost of doing so in zookeeper is
relatively low.

I'm not sure that we want to do that everywhere though.  An error
annotated by the function itself is less likely to get the message right
(case in point: the suggested error context for zkRemoveTree is "can't
clean up data", but that's not the only thing that zkRemoveTree is used
for - removing a service is not really "cleaning up data").

I suggest this: for every error return in a given function f, look at
where the error is coming from and determine if it is likely to make
sense when returned from f.

For example in this function, AddMachine, we've two error returns.
One comes from zk.Create. We know that zookeeper annotates its error
messages, and that the path ("/machines/machine-") will be understandable
even when returned from AddMachine, so we can use that error unadorned.

The other error comes from retryTopologyChange.  This error can be as
a result of topology.AddMachine, an unexpected zookeeper error reading,
creating or setting /topology, or failing to parse /topology.

retryTopologyChange itself doesn't have the necessary context to be able
to produce a decent error message (it doesn't know that it is being used
to add a machine), so I'd suggest adding the context "cannot add machine
to topology" here. Then the returned error will always make sense in the
context of AddMachine, regardless of why retryTopologyChange has failed.

I propose that if in doubt, we annotate errors, while being open to
the argument that in any particular case, the error *will* make sense
unadorned and thus can be returned as-is.

[niemeyer, 2012-06-13 12:32:37]

Looks much better. Some changes done are unnecessary, though. Please see the
ideas here and review the diff to see what has to be moved back.

https://codereview.appspot.com/6296064/diff/4001/state/state.go
File state/state.go (left):

https://codereview.appspot.com/6296064/diff/4001/state/state.go#oldcode171
state/state.go:171: return nil, err
Please revert all of these changes. The original "return nil, err" was correct
across the board, and will work fine with errorContext. With this change,
AddService is now returning a service even when there are errors, which we don't
want.

https://codereview.appspot.com/6296064/diff/4001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6296064/diff/4001/state/state.go#newcode134
state/state.go:134: stch, err = newCharm(s, curl, data)
Please revert. The original version is fine.

https://codereview.appspot.com/6296064/diff/4001/state/state.go#newcode159
state/state.go:159: stch, err = newCharm(s, curl, data)
Ditto.

https://codereview.appspot.com/6296064/diff/4001/state/state.go#newcode218
state/state.go:218: return fmt.Errorf("can't remove unit %q: %v", unit.Name(),
err)
No need to adorn it further. RemoveUnit itself will be taking care of that.

https://codereview.appspot.com/6296064/diff/4001/state/state.go#newcode232
state/state.go:232: if err = zkRemoveTree(s.zk, svc.zkPath()); err != nil {
Please revert.

https://codereview.appspot.com/6296064/diff/4001/state/state.go#newcode246
state/state.go:246: return nil, err
This error may come out as "can't find service key service-00000042".
errorContext should be used here too.

https://codereview.appspot.com/6296064/diff/4001/state/state.go#newcode260
state/state.go:260: var name string
Please revert and have "return nil, err" below.

https://codereview.appspot.com/6296064/diff/4001/state/state.go#newcode307
state/state.go:307: if _, err := s.zk.Create(path, "", 0, zkPermAll); err != nil
{
Please revert.

[niemeyer, 2012-06-13 12:39:04]

On 2012/06/13 12:12:37, rog wrote:
> I'm not sure that we want to do that everywhere though.  An error

I'm not suggesting we should do it everywhere, but we've spent an awful lot of
time talking about error messages already and the state package is all over the
place with messy error handling. I've heard complaints going in both directions
(more context! less context!) and no concrete way to address the problem. It's
clear to me that "look where the error comes from, and when in doubt do X, but
then consider Y, and be open about Z" won't get us anywhere good right now.

Let's take a stab at it now, consistently, specifically in the API surface of
the state package, and let's see how the result looks like. The changes are
minimal.

[TheMue, 2012-06-13 12:54:34]

Please take a look.

https://codereview.appspot.com/6296064/diff/4001/state/state.go
File state/state.go (left):

https://codereview.appspot.com/6296064/diff/4001/state/state.go#oldcode171
state/state.go:171: return nil, err
On 2012/06/13 12:32:38, niemeyer wrote:
> Please revert all of these changes. The original "return nil, err" was correct
> across the board, and will work fine with errorContext. With this change,
> AddService is now returning a service even when there are errors, which we
don't
> want.

Done.

https://codereview.appspot.com/6296064/diff/4001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6296064/diff/4001/state/state.go#newcode134
state/state.go:134: stch, err = newCharm(s, curl, data)
On 2012/06/13 12:32:38, niemeyer wrote:
> Please revert. The original version is fine.

Done.

https://codereview.appspot.com/6296064/diff/4001/state/state.go#newcode159
state/state.go:159: stch, err = newCharm(s, curl, data)
On 2012/06/13 12:32:38, niemeyer wrote:
> Ditto.

Done.

https://codereview.appspot.com/6296064/diff/4001/state/state.go#newcode218
state/state.go:218: return fmt.Errorf("can't remove unit %q: %v", unit.Name(),
err)
On 2012/06/13 12:32:38, niemeyer wrote:
> No need to adorn it further. RemoveUnit itself will be taking care of that.

Done.

https://codereview.appspot.com/6296064/diff/4001/state/state.go#newcode232
state/state.go:232: if err = zkRemoveTree(s.zk, svc.zkPath()); err != nil {
On 2012/06/13 12:32:38, niemeyer wrote:
> Please revert.

Done.

https://codereview.appspot.com/6296064/diff/4001/state/state.go#newcode246
state/state.go:246: return nil, err
On 2012/06/13 12:32:38, niemeyer wrote:
> This error may come out as "can't find service key service-00000042".
> errorContext should be used here too.

Done.

https://codereview.appspot.com/6296064/diff/4001/state/state.go#newcode260
state/state.go:260: var name string
On 2012/06/13 12:32:38, niemeyer wrote:
> Please revert and have "return nil, err" below.

Done.

https://codereview.appspot.com/6296064/diff/4001/state/state.go#newcode307
state/state.go:307: if _, err := s.zk.Create(path, "", 0, zkPermAll); err != nil
{
On 2012/06/13 12:32:38, niemeyer wrote:
> Please revert.

Done.

[rog, 2012-06-13 13:56:51]

On 13 June 2012 13:39,  <n13m3y3r@gmail.com> wrote:
> It's clear to me that "look where the error
> comes from, and when in doubt do X, but then consider Y, and be open
> about Z" won't get us anywhere good right now.

It seems to me that what we're introducing now is exactly a
stack trace of functions - each function has its own context
which is added to the context of those it calls. I thought we
were trying to avoid that, hence "consider Y".

Perhaps this issue is analogous to calling conventions - either
the caller or callee saves the registers/adds the context,
and each convention has its own advantages or disadvantages.

When the caller adds the context, it's more appropriate for the caller,
but the down side is that many calls might all need to add a similar context.

> Let's take a stab at it now, consistently, specifically in the API
> surface of the state package, and let's see how the result looks like.
> The changes are minimal.

I'm ok to go with the current idea, but I'm really not very keen
on errorContext - it clutters the non-error path of the function
and means it's harder to return custom error messages when
desired. That said, if we go for callee-added context,
I don't see a great alternative.

https://codereview.appspot.com/6296064/diff/2003/state/util.go
File state/util.go (right):

https://codereview.appspot.com/6296064/diff/2003/state/util.go#newcode293
state/util.go:293: func errorContext(err *error, format string, args
...interface{}) {
could we change this to something like this?

// errorContextf prefixes any error stored in err with text formatted
// according to the format specifier.  If err does not contain an error,
// errorContextf does nothing.
func errorContextf(err *error, format string, args ...interface{}) {
	if *err != nil {
		*err = errors.New(fmt.Sprintf(format, args...) + ": " + (*err).Error())
	}
}

That way it is amenable to automatic format checking with govet.

[niemeyer, 2012-06-13 14:07:23]

https://codereview.appspot.com/6296064/diff/2003/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6296064/diff/2003/state/state.go#newcode32
state/state.go:32: if path, err = s.zk.Create("/machines/machine-", "",
zookeeper.SEQUENCE, zkPermAll); err != nil {
Please revert the change here too. The original was fine and works with the
errorContext logic.

https://codereview.appspot.com/6296064/diff/2003/state/state.go#newcode146
state/state.go:146: return nil, err
return fmt.Errorf("charm not found")

There's a real meaning for the application for this node to not exist, and the
error message was clearly explaining it. A ZooKeeper ZNONODE error means
nothing, though.

https://codereview.appspot.com/6296064/diff/2003/state/state.go#newcode310
state/state.go:310: return nil, nil, fmt.Errorf("relation between %s and %s is
invalid", endpoints[0], endpoints[1])
Again, the old error messages were better. We can preserve the old messages
here, and add the errorContext below only. We need to watch out for not using
this trick blindly when we can clearly do better.

You can easily preserve the original messages here, and add errorContext below
instead.

https://codereview.appspot.com/6296064/diff/2003/state/state.go#newcode314
state/state.go:314: }
Here, more precisely.

https://codereview.appspot.com/6296064/diff/2003/state/util.go
File state/util.go (right):

https://codereview.appspot.com/6296064/diff/2003/state/util.go#newcode293
state/util.go:293: func errorContext(err *error, format string, args
...interface{}) {
On 2012/06/13 13:56:51, rog wrote:
> That way it is amenable to automatic format checking with govet.

Sure, but only if we can see go vet actually working with this, otherwise this
is just making the function ugly for no good reason. Do you have a
self-contained example of it working?

The documentation you provided is better no matter what, though.

[TheMue, 2012-06-13 14:58:05]

Please take a look.

https://codereview.appspot.com/6296064/diff/2003/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6296064/diff/2003/state/state.go#newcode32
state/state.go:32: if path, err = s.zk.Create("/machines/machine-", "",
zookeeper.SEQUENCE, zkPermAll); err != nil {
On 2012/06/13 14:07:23, niemeyer wrote:
> Please revert the change here too. The original was fine and works with the
> errorContext logic.

Done. Ooops, missed it.

https://codereview.appspot.com/6296064/diff/2003/state/state.go#newcode146
state/state.go:146: return nil, err
On 2012/06/13 14:07:23, niemeyer wrote:
> return fmt.Errorf("charm not found")
> 
> There's a real meaning for the application for this node to not exist, and the
> error message was clearly explaining it. A ZooKeeper ZNONODE error means
> nothing, though.

Done.

https://codereview.appspot.com/6296064/diff/2003/state/state.go#newcode310
state/state.go:310: return nil, nil, fmt.Errorf("relation between %s and %s is
invalid", endpoints[0], endpoints[1])
On 2012/06/13 14:07:23, niemeyer wrote:
> Again, the old error messages were better. We can preserve the old messages
> here, and add the errorContext below only. We need to watch out for not using
> this trick blindly when we can clearly do better.
> 
> You can easily preserve the original messages here, and add errorContext below
> instead.

Done.

https://codereview.appspot.com/6296064/diff/2003/state/state.go#newcode314
state/state.go:314: }
On 2012/06/13 14:07:23, niemeyer wrote:
> Here, more precisely.

Done.

https://codereview.appspot.com/6296064/diff/2003/state/util.go
File state/util.go (right):

https://codereview.appspot.com/6296064/diff/2003/state/util.go#newcode293
state/util.go:293: func errorContext(err *error, format string, args
...interface{}) {
On 2012/06/13 14:07:23, niemeyer wrote:
> On 2012/06/13 13:56:51, rog wrote:
> > That way it is amenable to automatic format checking with govet.
> 
> Sure, but only if we can see go vet actually working with this, otherwise this
> is just making the function ugly for no good reason. Do you have a
> self-contained example of it working?
> 
> The documentation you provided is better no matter what, though.

Done.

[niemeyer, 2012-06-13 15:03:44]

LGTM, with the minor fix:

https://codereview.appspot.com/6296064/diff/5006/state/util.go
File state/util.go (right):

https://codereview.appspot.com/6296064/diff/5006/state/util.go#newcode294
state/util.go:294: func errorContext(err *error, format string, args
...interface{}) {
Either the function name has to change, or the documentation has to be fixed.

[TheMue, 2012-06-13 15:52:16]

*** Submitted:

state: Second approach to improve the error handling.

This second approach returns error generated with fmt.Errorf()
and containing high-level informations like names. In case of
an internal error this is appended or, in case of some topology
errors, directly returned. This depends on the returned error
of the topology method.

R=niemeyer, rog
CC=
https://codereview.appspot.com/6296064