Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spanner: cannot wrap error for ReadWriteTransaction #1223

Open
kazegusuri opened this issue Nov 17, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@kazegusuri
Copy link

commented Nov 17, 2018

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 #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

This comment has been minimized.

Copy link
Collaborator

commented Nov 19, 2018

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

This comment has been minimized.

Copy link
Author

commented Nov 20, 2018

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

This comment has been minimized.

Copy link
Collaborator

commented Nov 20, 2018

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

@tenntenn

This comment has been minimized.

Copy link

commented Mar 29, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.