/juju

apiserver/params: remove ClientError helper #4361

Merged
merged 1 commit into from Feb 11, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@davecheney @howbazaar @jujubot
@davecheney
Contributor

davecheney commented Feb 10, 2016

Updates LP#1538583

api/apiclient.APICall was the sole caller of params.ClientError.
params.Client error would rewrite the error value iff it was
rpc.RequestError, claiming that it was leaking unnecessary information
to the caller, specfically the "request error: " prefix. In b4e2785
the error prefix was convered to an errors.Annotate wrapper removing
the requirement to convert the error type.

Now, by removing params.ClientError, we can also remove the
errors.Annotate wrapper as the type of the error tells us what it is.

So, callers get

a. A nice annotated backtrace using errors.Trace
b. Access to the original *rpc.RequestError to inspect if they want.
rpc/server.go
@@ -4,7 +4,7 @@
package rpc
import (
- "fmt"
+ "errors"
@howbazaar

howbazaar Feb 11, 2016

Owner

could always use "github.com/juju/errors" here instead.

@davecheney

davecheney Feb 11, 2016

Contributor

fixed

@howbazaar
Owner

howbazaar commented Feb 11, 2016

LGTM, one suggestion you may want to look at.
@davecheney
Contributor

davecheney commented Feb 11, 2016

$$JFDI$$
@jujubot
Contributor

jujubot commented Feb 11, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju
@jujubot
Contributor

jujubot commented Feb 11, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/6348
@davecheney
apiserver/params: remove ClientError helper 
Updates LP#1538583

api/apiclient.APICall was the sole caller of params.ClientError.
params.Client error would rewrite the error value iff it was
rpc.RequestError, claiming that it was leaking unnecessary information
to the caller, specfically the "request error: " prefix. In b4e2785
the error prefix was convered to an errors.Annotate wrapper removing
the requirement to convert the error type.

Now, by removing params.ClientError, we can also remove the
errors.Annotate wrapper as the _type of the error_ tells us what it is.

So, callers get

a. A nice annotate error backtrace using errors.Trace
b. Access to the original *rpc.RequestError to inspect if they want.
e11ae23
@davecheney
Contributor

davecheney commented Feb 11, 2016

$$JFDI$$
@jujubot
Contributor

jujubot commented Feb 11, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

jujubot added a commit that referenced this pull request Feb 11, 2016

@jujubot
Merge pull request #4361 from davecheney/apiserver-params-remove-clie… 
…nterror

apiserver/params: remove ClientError helper

Updates LP#1538583

api/apiclient.APICall was the sole caller of params.ClientError.
params.Client error would rewrite the error value iff it was
rpc.RequestError, claiming that it was leaking unnecessary information
to the caller, specfically the "request error: " prefix. In b4e2785
the error prefix was convered to an errors.Annotate wrapper removing
the requirement to convert the error type.

Now, by removing params.ClientError, we can also remove the
errors.Annotate wrapper as the _type of the error_ tells us what it is.

So, callers get

a. A nice annotated backtrace using errors.Trace
b. Access to the original *rpc.RequestError to inspect if they want.
f1ca8c7

@jujubot jujubot merged commit f1ca8c7 into juju:master Feb 11, 2016

@davecheney davecheney deleted the davecheney:apiserver-params-remove-clienterror branch Feb 11, 2016

cherylj pushed a commit to cherylj/juju that referenced this pull request Feb 15, 2016

lp1544796: Fix error checking in Restore 
Bug 1544796 was introduced with PR
juju#4361, which
changed how errors are returned from the rpc
calls.  This spot where the error was compared
to rpc.ErrShutdown was not updated to use
errors.Cause() as in other places.

There were some other logic errors in the restore
code which needed to be addressed:

1 - The check for an "upgrade in progress" was wrong.
That error comes from the rpc call, and not as the
result from the method on the facade being called.  So,
the check should be if err is an "upgrade in progress"
error, not remoteError.  We also cannot use
params.IsCodeUpgradeInProgress as that will not work
on the rpc.RequestError error type that is returned.
The check now uses the same method the bootstrap command
does.

2 - In restore() and finishRestore(), err was
re-declared inside the attempt strategy block, but then
used outside of the block where it would always have
a nil value.
4ec11be

@cherylj cherylj referenced this pull request Feb 15, 2016

Merged

lp1544796: Fix error checking in Restore #4421

jujubot added a commit that referenced this pull request Feb 15, 2016

@jujubot
Merge pull request #4421 from cherylj/fix_restore_shutdown_check 
lp1544796: Fix error checking in Restore

Bug 1544796 was introduced with PR
#4361, which
changed how errors are returned from the rpc
calls.  This spot where the error was compared
to rpc.ErrShutdown was not updated to use
errors.Cause() as in other places.

There were some other logic errors in the restore
code which needed to be addressed:

1 - The check for an "upgrade in progress" was wrong.
That error comes from the rpc call, and not as the
result from the method on the facade being called.  So,
the check should be if err is an "upgrade in progress"
error, not remoteError.  We also cannot use
params.IsCodeUpgradeInProgress as that will not work
on the rpc.RequestError error type that is returned.
The check now uses the same method the bootstrap command
does.

2 - In restore() and finishRestore(), err was
re-declared inside the attempt strategy block, but then
used outside of the block where it would always have
a nil value.

(Review request: http://reviews.vapour.ws/r/3861/)
e835f63
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment