Google-cloud-go: spanner: cannot wrap error for ReadWriteTransaction

[kazegusuri]

spanner client uses spanner.Error for error inside library and spanner server. ReadWriteTransaction assumes spanner.Error is returned when an error happens so it gets reasons of the error and retry info from grpc status and trailer. It is an explict value. If we wraps the error before returning into ReadWriteTransaction, it cannot get the reasons from the error.
Fortunately it uses status.Code() to get grpc status code of the error, so we can still return exact grpc status code by implementing GRPCStatus() interface in the wrapped error.
But the trailer cannot be handled outside this library. As a result, backoff delay will be 1 second at least when abort happens.

This is related to https://github.com/GoogleCloudPlatform/google-cloud-go/issues/1215, but probably we have some options to fix this.

1. use an interface to get trailers from spanner.Error. A wrapped error should implement the interface to return trailers.
2. make default backoff configurable.

[jadekler]

I'm having a bit of a hard time figuring out the request here. Could you tell me a little about what you're trying to accomplish? And then, could you describe what you're doing, what you'd like to happen, and what's happening instead?

[kazegusuri]

I want to wrap errors happened inside ReadWriteTransaction like this.

import (
    "github.com/pkg/errors"
)

_, err = client.ReadWriteTransaction(ctx, func(ctx context.Context, tx *spanner.ReadWriteTransaction) error {
    _, err := tx.ReadRow(ctx, "Foo", spanner.Key{"foo"}, []string{...})
    if err != nil {
        return errors.Wrap(err, "ReadRow failed") // wrap error
    }
    ...
    return nil
}

There are problems to wrap errors.

The ReadWriteTransaction handles retriable errors for spanner for read,query,commit when spanner returns grpc status code like Unavaiable, Unknown, Abort. But wrapped error mimics the grpc status code from ReadWriteTransaction, which results in client libarary never handle retriable errors.

The error handling inside ReadWriteTransaction uses grpc.Code() to get status code here, so If the wrapped error implements GRPCStatus() *status.Status interface, the ReadWriteTransaction can see the status code properly from the wrapped error.

The second problem is backoff for retry. When abort happens, ReadWriteTransaction tries to retry with backoff. The backoff delays respect RetryInfo in trailers retrurned from spanner. But once we wrap errors, there is no way to pass trailers to ReadWriteTransaction. If ReadWriteTransaction cannot find trailer from errors, backoff delays depend on defaultBackoff. The default backoff begins 1 second delay. It's too long and causes huge performance degradation.

I want at least to make the delays configurable for retry by setting the default backoff (option 2). Or implements an interface to pass trailers in order to ReadWriteTransaction properly sees RetryInfo from wrraped errors (option 1).

[jadekler]

Ahhh. Thank you for this excellent description. I'll label this a feature request - this will take some discussing.

[tenntenn]

I develop a static analysis tool for Cloud Spanner, named zagane.
zagane can find wrapped errors.
It is useful until the issue is fixed.
https://github.com/gcpug/zagane

[dfinkel]

Now that go 1.13's error-unwrapping has been out for a couple months, it seems like it would be ideal to either use [errors.As] if the type-assertion fails or iteratively call Unwrap() if possible in toSpannerErrorWithMetadata() here:

https://github.com/googleapis/google-cloud-go/blob/f9fc089da7b416e5cfb30c7f0bc9ce73fc68ed20/spanner/errors.go#L78-L87

It might be necessary to move that function to its own set of files with build-tags for go 1.13 and higher, etc.

For older go versions, golang.org/x/xerrors can be used as per #1310

[iwata]

@olavloite
Is there any progress on this issue?

[olavloite]

@iwata Yes, we are currently working on it here: https://code-review.googlesource.com/c/gocloud/+/48730