-
Notifications
You must be signed in to change notification settings - Fork 27
feat: support statement_timeout and transaction_timeout property #578
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
Conversation
80dbaa0 to
0d69f22
Compare
1791559 to
4667c42
Compare
1b7b5f0 to
1fc9908
Compare
Add a statement_timeout connection property that is used as the default timeout for the execution of all statements that are executed on a connection. The timeout is only used for the actual execution, and not attached to the iterator that is returned for a query. This also means that a query that is executed without the DirectExecuteQuery option, will ignore the statement_timeout value. Also adds a transaction_timeout property that is additionally used for all statements in a read/write transaction. The deadline of the transaction is calculated at the start of the transaction, and all statements in the transaction get this deadline, unless the statement already has an earlier deadline from for example a statement_timeout or a context deadline. This change also fixes some issues with deadlines when using the gRPC API of SpannerLib. The context that is used for an RPC invocation is cancelled after the RPC has finished. This context should therefore not be used as the context for any query execution, as the context is attached to the row iterator, and would cancel the query execution halfway. Fixes #574 Fixes #575
1fc9908 to
5ae4187
Compare
conn.go
Outdated
| // one error. This will preserve the DeadlineExceeded error code from statementErr, and include the request | ||
| // ID from the Spanner error. | ||
| s := status.FromContextError(statementErr) | ||
| return fmt.Errorf("%w: %w", s.Err(), res.dirtyErr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you check this... fmt.Errorf allows at most one %w verb. Using two %w will cause a runtime panic: "fmt: multiple %w verbs in format".
return fmt.Errorf("%v: %w", s.Err(), res.dirtyErr)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... Interesting point. This was actually changed in Go 1.20: https://go.dev/doc/go1.20#errors
So anyone using the correct minimum version of Go should not run into that problem. But a safer way to solve this might be to just use the errors.Join function, as that would then give a compile-time error instead of a runtime panic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM...
…gleapis#578) * feat: support statement_timeout and transaction_timeout property Add a statement_timeout connection property that is used as the default timeout for the execution of all statements that are executed on a connection. The timeout is only used for the actual execution, and not attached to the iterator that is returned for a query. This also means that a query that is executed without the DirectExecuteQuery option, will ignore the statement_timeout value. Also adds a transaction_timeout property that is additionally used for all statements in a read/write transaction. The deadline of the transaction is calculated at the start of the transaction, and all statements in the transaction get this deadline, unless the statement already has an earlier deadline from for example a statement_timeout or a context deadline. This change also fixes some issues with deadlines when using the gRPC API of SpannerLib. The context that is used for an RPC invocation is cancelled after the RPC has finished. This context should therefore not be used as the context for any query execution, as the context is attached to the row iterator, and would cancel the query execution halfway. Fixes googleapis#574 Fixes googleapis#575 * chore: use errors.Join instead of two %w verbs
Add a statement_timeout connection property that is used as the default timeout
for all statements that are executed on a connection. If both a context timeout
and a statement_timeout is specified, then the lower of the two will be used.
Also adds a transaction_timeout property that is additionally used for all
statements in a read/write transaction. The deadline of the transaction is
calculated at the start of the transaction, and all statements in the transaction
get this deadline, unless the statement already has an earlier deadline from
for example a statement_timeout or a context deadline.
Fixes #574
Fixes #575