feat(spanner): support commit stats

[hengfengli]

Current approach

To not make a breaking change, I came up with the following changes (in this draft PR):

• Add a struct TransactionOptions
  • Consider more changes in future, so I made a new struct to include CommitOptions as a field
• Add functions Client.ReadWriteTransactionWithOptions and NewReadWriteStmtBasedTransactionWithOptions and TransactionOptions will be passed into these two functions
• Add structs CommitOptions and CommitResponse
• ReadWriteTransactionWithOptions and CommitReturnResponse will return CommitResponse instead of time.Time

Alternative approach

Another option is to update the struct ReadWriteTransaction:

type ReadWriteTransaction struct {
    ...
    ReturnCommitStats bool
}

Then, update Client.ReadWriteTransaction and ReadWriteStmtBasedTransaction.Commit to return (CommitResponse, error).

Basically, the ReturnCommitStats option can be set on a transaction struct inside the passing function:

_, err := client.ReadWriteTransaction(ctx, func(ctx context.Context, tx *ReadWriteTransaction) error {
	tx.ReturnCommitStats = true
        ...
})

Of course, this would be a breaking change for users.

[hengfengli]

@skuruppu @olavloite Could you please give me some advice on this change? Which one you think is better? Or, if you have any other suggestions? I appreciate.

[olavloite]

@skuruppu @olavloite Could you please give me some advice on this change? Which one you think is better? Or, if you have any other suggestions? I appreciate.

This is a quite difficult choice to make. My initial thoughts are:

1. A solution where Client.ReadWriteTransaction(..) and ReadWriteStmtBasedTransaction.Commit(..) return a CommitResponse instead of a time.Time would definitely have been my preferred choice if this was an initial design. This solution reduces the number of overloads, and allows for future extensions of the CommitResponse without breaking anything.
2. Client.ReadWriteTransaction(..) is a method that is (most probably) used by a lot, which means making a breaking change to that method will require a lot of code changes to be made by client using the library. The only mitigating factor here might be that many users could be ignoring the commit timestamp return value, which would mean that those occurrences will not need to be changed.

Based on the above, and partly on the assumption that many users are currently ignoring the commit timestamp that is returned by Client.ReadWriteTransaction, I would lean towards the following solution:

1. Change the return type of Client.ReadWriteTransaction(..) and ReadWriteStmtBasedTransaction.Commit(..) to CommitReponse. This is a breaking change, and will require everyone that does anything with the currently returned commit timestamp to change their code. The advantage is however that we have a struct that could easily be extended with additional fields if the backend were to return additional information in the future.
2. Add a TransactionOption interface and add that as a vararg argument to the Client.ReadWriteTransaction(..) and NewReadWriteStmtBasedTransaction(..) methods.

The signature of the two methods would then be:

• Client.ReadWriteTransaction(ctx context.Context, f func(context.Context, *ReadWriteTransaction) error, opts ...TransactionOption) (response CommitResponse, err error)
• NewReadWriteStmtBasedTransaction(ctx context.Context, c *Client, opts ...TransactionOption) (*ReadWriteStmtBasedTransaction, error)

@hengfengli @skuruppu
What are your feeling/assumption regarding the number of users currently ignoring the return commit timestamp? If most users ignore it, then this change is less intrusive than if most users do use it.

[skuruppu]

I completely agree that if we were designing from scratch, we should just accept the request proto as input and set the return value to the response proto. I think we should consider this when supporting new methods just to avoid having to make this type of decision in the future.

But since we have to make this decision, I'm quite worried about making a breaking change to these commonly used methods. I don't have any data but if people are reading the commit timestamp, then they are likely to be using it for some important reason so I would hesitate to break whatever that use case may be.

So @thiagotnunes and I were actually discussing about making the Java change similar to what @hengfengli has proposed here where we introduce a XWithOptions() overload that takes in varargs and returns a CommitResponse. The XWithOptions() will also help with a couple of the other changes where we'll be introducing more configuration options.

I don't know if we need to go even further and create a struct for the return type with CommitResponse being a field, in case we decide to return other fields in the future but maybe I'm being paranoid.

I know it's not the ideal solution we want but I think what @hengfengli has produced is the least invasive option. Especially given that users are only meant to use this new option to debug queries. I wouldn't want a debugging change to affect existing code running in production apps.

[hengfengli]

@olavloite Thanks for the ideas. I was inspired by adding TransactionOption to NewReadWriteStmtBasedTransaction. As @skuruppu's saying, we hope to keep this as a least breaking change for users (also, having the flexibility to add more options in the future). I updated my code according to your suggestion. Please take another look.

For the method CommitReturnResponse, we can remove it by changing Commit's return type to (CommitResponse, error). This would be a breaking change, but my assumption is that many people are not using NewReadWriteStmtBasedTransaction.

[hengfengli]

I have created a more generic API change to support transaction options in #3058. Please review the code over there, because this PR will be based on it.

[hengfengli]

@olavloite @skuruppu This PR is ready for review. Please take a look.

[skuruppu]

@olavloite since @hengfengli is away, would you mind removing references to overload_delay in this PR? I think you should be able to push commits to the same branch. But if not, feel free to fork the PR.

[olavloite]

Replaced by #3444