feat(firestore): add WithCommitResponseTo TransactionOption

[galenwarren]

Adds a new TransactionOption -- GetCommitTime -- that allows the caller to specify the address of a time.Time where the commit time of the transaction should be written, upon successful commit.

[google-cla[bot]]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[galenwarren]

@telpirion @enocom I was just checking on this. Would either of you be able to provide a code review? Thanks.

[enocom]

Sorry for the slow response @galenwarren. Ownership of this library has changed in the past few months. Let me reach out to some folks and see who should be looking at this.

[galenwarren]

Thanks!

On Thu, Mar 30, 2023, 11:02 AM Eno Compton @.***> wrote:

Sorry for the slow response @galenwarren https://github.com/galenwarren. Ownership of this library has changed in the past few months. Let me reach out to some folks and see who should be looking at this.

— Reply to this email directly, view it on GitHub https://github.com/googleapis/google-cloud-go/pull/6967#issuecomment-1490464807, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAF7RBK7HINDLQSRFBEKYTDW6WOBRANCNFSM6AAAAAARST3DG4 . You are receiving this because you were mentioned.Message ID: @.***>

[enocom]

cc @bhshkh and @kolea2 who might know who should look at this.

[galenwarren]

@enocom @bhshkh @kolea2 Any update here? Thanks.

[bhshkh]

Reviewing

[galenwarren]

@bhshkh Thanks, please let me know if you have any questions or suggestions.

[galenwarren]

@bhshkh Hi, just checking in here. Is there any update? Thanks.

[bhshkh]

Under discussion internally. Will update

[galenwarren]

@bhshkh Is there anything I can do here to help?

[galenwarren]

Is this one not going to get merged?

[codyoss]

@bhshkh is this something we want to support?

[galenwarren]

If anyone would like me to rebase, just let me know. Thanks!

[bhshkh]

Sorry for the very delayed response. Reviewing this

[bhshkh]

One problem I see with the current design is that it is not extendable. This is what the commit response looks like

// The response for [Firestore.Commit][google.firestore.v1.Firestore.Commit].
type CommitResponse struct {
	state         protoimpl.MessageState
	sizeCache     protoimpl.SizeCache
	unknownFields protoimpl.UnknownFields

	// The result of applying the writes.
	//
	// This i-th write result corresponds to the i-th write in the
	// request.
	WriteResults []*WriteResult `protobuf:"bytes,1,rep,name=write_results,json=writeResults,proto3" json:"write_results,omitempty"`
	// The time at which the commit occurred. Any read with an equal or greater
	// `read_time` is guaranteed to see the effects of the commit.
	CommitTime *timestamppb.Timestamp `protobuf:"bytes,2,opt,name=commit_time,json=commitTime,proto3" json:"commit_time,omitempty"`
}

Apart from CommitTime field, there is another field called WriteResults. If/when in the future, we want to support retrieving WriteResults, we will have to introduce another transaction option and so on and so forth for any more fields added to the above struct.

Another thing is that the name GetCommitTime does not look appropriate. Open to any alternatives to that. One option could be to rename the transaction option to WithCommitResponseTo(*pb.CommitResponse)

[bhshkh]

To minimize friction, consider setting Allow edits from maintainers on the PR, which will enable project committers and automation to update your PR.

[bhshkh]

Found similar request: https://github.com/googleapis/google-cloud-go/issues/1412

[bhshkh]

@bhshkh is this something we want to support?

Yes

[galenwarren]

@bhshkh Thanks for looking at this.

Regarding the design -- agreed, it would be better if RunTransaction returned both a result (which could hold the CommitTime and the WriteResults) and an error, instead of just an error. But that would be a breaking change. I suppose a new RunTransaction (RunTransaction2, RunTransactionEx?) method could be added with a new signature, would that be better?

I'm happy to enable Allow edits from maintainers but I can't seem to find the option to select. Should I see that in the right-hand column somewhere?

WithCommitResponseTo sounds good to me.

[bhshkh]

@bhshkh Thanks for looking at this.

Regarding the design -- agreed, it would be better if RunTransaction returned both a result (which could hold the CommitTime and the WriteResults) and an error, instead of just an error. But that would be a breaking change. I suppose a new RunTransaction (RunTransaction2, RunTransactionEx?) method could be added with a new signature, would that be better?

I'm happy to enable Allow edits from maintainers but I can't seem to find the option to select. Should I see that in the right-hand column somewhere?

WithCommitResponseTo sounds good to me.

Instead of having a new function, we could create a new transaction option as you originally suggested. In the original design, *time.time was getting passed as argument to transaction option. Instead, use *CommitResponse as argument.

CommitResponse could be a wrapper over pb.CommitResponse. (General design philosophy is not to expose the protos. In the handwritten layer of Firestore, there are no exported functions taking raw protos as function argument or returning raw protos.)

To allow edits, see point 4 at https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

[galenwarren]

Regarding "Allow edits and access secrets my maintainers": Yes, that's what I tried before. I just don't get a checkbox to enable it anywhere. Could it be because this PR is old somehow? Not sure ... it's strange.

I see what you're suggesting about CommitResponse, makes sense. I'll make the changes.

[galenwarren]

@bhshkh Just pushed some changes, please let me know what you think. Thanks.

[galenwarren]

@bhshkh I saw the checks re: missing comments, I'll add them in the next commit where I address the other feedback. Will either be today or tomorrow.

[galenwarren]

@bhshkh I added the missing comments, so that should address the first failed check. I wasn't sure quite what to make of the second failed check, it wasn't clear to me what it was saying, and it seemed to be wrt bigquery. I did rebase again with this last commit so maybe I've pulled in some new code related to that ...

[galenwarren]

@jba @bhshkh Just wondering if we're still moving forward here?

[bhshkh]

@jba @bhshkh Just wondering if we're still moving forward here?

Thanks for the modifications! Yes. Waiting on @jba's review.

[galenwarren]

@bhshkh @jba Thanks, let me know if there's anything I need to do.

[jba]

Sorry I dropped the ball on this. Looking now.

[galenwarren]

Thanks @jba and @bhshkh .

What's the next step here, I'm not quite sure what this means:

Waiting on code owner review from googleapis/yoshi-go-admins