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]

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 <#6967 (comment)>, 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]

I'm happy to enable Allow edits from maintainers but I can't seem to find the option to select.

Side note: Found the reason. #10824 (comment)

[galenwarren]

@bhshkh @jba @codyoss Thanks all.

The only things that still seem to be failing are API checks against other parts of the project, I'm guessing those are due to outdated code in this branch, so I kicked off a merge to main.

What are the next steps, anything I need to do?

[galenwarren]

@bhshkh @jba @codyoss What are the next steps here? Thanks.

[bhshkh]

Sorry for late response.
Forced kokoro run and enabled auto merge
Will monitor

[bhshkh]

Unable to merge. This branch is out-of-date with the base branch

[bhshkh]

I'm happy to enable Allow edits from maintainers but I can't seem to find the option to select.

Side note: Found the reason. #10824 (comment)

@galenwarren , can you please recreate this PR from personal fork so that I have permissions to update the branch ?

[galenwarren]

@bhshkh I'd really like to avoid recreating the PR if possible. I've invited you to (temporarily) be a collaborator (writer) on the project in my organization, does that get you what you need?

[bhshkh]

@bhshkh I'd really like to avoid recreating the PR if possible. I've invited you to (temporarily) be a collaborator (writer) on the project in my organization, does that get you what you need?

Sorry, I can't accept the invite

[galenwarren]

@bhshkh Do you mean it doesn't work or you're not allowed, by policy or something like that?

EDIT: Also, could I just push the "Update Branch" button for you, to sync to base branch?

[galenwarren]

@bhshkh Wasn't sure if you saw my suggestion that I just rebase to main for you?

[bhshkh]

I'm not allowed to. Please rebase. I'll actively monitor the PR. Thanks!

[galenwarren]

@bhshkh Understood, thanks. I just rebased to main.

[bhshkh]

Merged !

[galenwarren]

Excellent - thanks!

…

On Tue, Oct 1, 2024, 2:19 PM Baha Aiman ***@***.***> wrote: Merged ! — Reply to this email directly, view it on GitHub <#6967 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAF7RBMJBHUUFR7IC7U4GZ3ZZLRTRAVCNFSM6AAAAABJXVN642VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBWGY3TCOBZGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>