Revamp CreateCommits to just create Commits and add specialized Push and Rebase endpoints #43

saquibmian · 2023-11-30T16:17:00Z

There are two things going on in this PR:

CreateCommits is updated so that it simply creates Commits for Modules without pushing them onto any Branch or assigning any Tags to them.
- The idea is that Commits can be "uploaded" to the BSR before they are placed in their correct places in Branch history or assigned Tags. This is very useful for sync, where potentially 1000s of Commits need to be created before pushing them on a Branch. Because Branch history should be patched atomically and with a lock, creating Commits in-band becomes untenable.
- These Commits can only be referenced by their ID, Digest, or a VCSCommitRef if there was any AssociatedVCSCommit.
Two new RPCs are added to mutate a Branch's history: BranchService.Push and BranchService.Rebase.
- Push provides no assertions on the final state of a Branch's history. This is the model that buf push currently uses, where all concurrent writes win, but the last write is king and becomes the latest commit.
- Rebase allows the caller to assert on the final state of a Branch's history. Concurrent writes are idempotent as long as they assert the same final state. If they don't, the first write wins and all others lose. This is the model that buf sync will use.

buf/registry/module/v1beta1/branch_service.proto

saquibmian · 2023-11-30T16:18:12Z

buf/registry/module/v1beta1/branch_service.proto

+  // returned. Additionally, each patch in the request asserts the expected state of the branch. If
+  // the actual state does not match the expected state, the patch is rejected and an error is
+  // returned.
+  rpc PatchBranchHistories(PatchBranchHistoriesRequest) returns (PatchBranchHistoriesResponse);


I didn't put any idempotentcy annotations on these two RPCs yet. I think that PatchBranchHistories is IDEMPOTENT but I'm unclear on what PushCommits is. I also don't understand why CreateCommits is IDEMPOTENT... it will fail if invoked repeatedly.

buf/registry/module/v1beta1/branch_service.proto

buf/registry/module/v1beta1/commit_service.proto

buf/registry/module/v1beta1/branch_service.proto

buf/registry/module/v1beta1/commit_service.proto

buf/registry/module/v1beta1/branch_service.proto

bufdev · 2023-12-01T04:10:07Z

I updated with some edits - for ease of reference, here's the entire diff: 6d72e67...ad64ae8

Some main points:

RPCs are now Push and Rebase.
The concept of "head commit" now replaces the concept of "latest commit" - latest implies time, while "head" has clearer meaning.
PushRequest.Value now takes mulltiple commit_ids, thereby batching a single Branch update into a single Value. This gets rid of potential duplicates, and makes it possible to have the response length match the request length. I think it's also just clearer for the caller as to what is happening.
Push and Rebase now just return Branches, which are the only entity that actually gets modified I believe.
Operation, Operation.CreateBranch, Operation.UpdateBranch are now the names of the operations on RebaseRequest. CreateBranch and UpdateBranch now nested inside Operation.
missing_blobs moved top-level.
Various other naming updates.
Protobuf validation put in place for repeated fields.
BUF_VERSION updated to v1.28.1 to make sure we are incorporating PROTOVALIDATE rules (I can't remember what the commit points to, and good to update anyways).
Some extra documentation spec'ing out behavior.

Please double-check that I didn't miss anything and that the comments make sense and reflect the correct behavior, but assuming I didn't miss anything this is good to go from my end (but please triple-check).

buf/registry/module/v1beta1/branch_service.proto

akshayjshah · 2023-12-01T16:58:32Z

buf/registry/module/v1beta1/branch_service.proto

+  // a BranchRef that refers to the same Branch. An error will be returned if any
+  // two BranchRefs refer to the same Branch.
+  repeated Value values = 1 [(buf.validate.field).repeated.min_items = 1];
+}


@saquibmian do we need to support pushing to multiple branches simultaneously - push is last writer wins anyways, so maybe it's not worthwhile? If we got rid of that, we'd be able to simplify this RPC and get rid of the nested Value message entirely.

We do not, but not supporting batching is inconsistent with the rest of the schema. Every RPC today supports batching, except the List* RPCs (which in theory could support batching trivially).

buf/registry/module/v1beta1/branch_service.proto

nicksnyder · 2023-12-01T18:57:13Z

buf/registry/module/v1beta1/commit_service.proto

  //   - If a Digest is referenced, this is interpreted to mean the latest released Commit that has this Digest.
  //     Digests referencing unreleased Commits cannot be referenced.
+  //
+  // TODO: what is "latest released Commit" if we now have "head Commit"?


I think we need to re-evaluate everywhere in this API we use the word "released" given our new buf sync model

Is "released" a property of a BSR commit?

If a BSR commit can be on multiple branches, then I don't think we can make released a property on a BSR commit (because "released" only makes sense within the context of a history).

If a BSR commit can only ever be on a single branch, they it would maybe be ok to model it this way

Also need to take a look at branches service and probably rename references to "release branch" to "default branch"

Yes we need to re-evaluate this

Is this something we can do separate from this PR, or do we want to formalize "released" now and rope it into this PR?

I think we should try to land this PR and save this bigger TODO for a followup

Perfect, I'll leave this here for now and we can clean up/formalize "release" after this.

Co-authored-by: Akshay Shah <[email protected]>

Co-authored-by: Nick Snyder <[email protected]>

buf/registry/module/v1beta1/branch_service.proto

saquibmian · 2023-12-04T17:06:28Z

buf/registry/module/v1beta1/branch.proto

-  // The id of the latest Commit created on the Branch.
-  string latest_commit_id = 10 [
+  // The id of the head Commit created on the Branch.
+  string head_commit_id = 10 [
    (buf.validate.field).required = true,


I noticed this is required. Does this imply that an empty Branch does not exist, or should we relax this constraint?

Unless this causes a problem elsewhere, it makes sense to me that an empty branch doesn't exist because this is true in Git (a branch pointer has to point to a commit).

I updated the docs to reflect this.

buf/registry/module/v1beta1/branch_service.proto

nicksnyder · 2023-12-04T17:53:13Z

Please clean up PR description to reflect the actual implementation/names we landed on (and remove the TODO list). Basically make the PR description what the merged commit description should look like.

nicksnyder · 2023-12-04T22:20:18Z

Akshay lgtm'd in Slack and Peter told me live today that minor changes are fine and don't need his re-review, so I am going to click the merge button on this.

The only changes that happened since Peter took a look were docs changes and idempotency (which Akshay reviewed).

saquibmian added 2 commits November 29, 2023 11:58

Proposal: atomic branch history RPCs and branchless push

b72837d

Update comments and RPC name

a301ca1

saquibmian requested a review from bufdev as a code owner November 30, 2023 16:17

saquibmian commented Nov 30, 2023

View reviewed changes

buf/registry/module/v1beta1/branch_service.proto Outdated Show resolved Hide resolved

saquibmian commented Nov 30, 2023

View reviewed changes

buf/registry/module/v1beta1/branch_service.proto Outdated Show resolved Hide resolved

saquibmian commented Nov 30, 2023

View reviewed changes

buf/registry/module/v1beta1/commit_service.proto Show resolved Hide resolved

Fix lint

4ec3aec

saquibmian commented Nov 30, 2023

View reviewed changes

buf/registry/module/v1beta1/branch_service.proto Outdated Show resolved Hide resolved

saquibmian commented Nov 30, 2023

View reviewed changes

buf/registry/module/v1beta1/commit_service.proto Outdated Show resolved Hide resolved

bufdev reviewed Nov 30, 2023

View reviewed changes

buf/registry/module/v1beta1/branch_service.proto Outdated Show resolved Hide resolved

saquibmian and others added 13 commits November 30, 2023 14:30

Add repeated associated_vcs_commits back... that did not mean to change

6d72e67

commit

cacb418

s/Push/Append/ and adjust docs

0ae1cf3

TODO

cbc577e

docs

f623f85

TODO

b2255fb

remove optional

988975a

TODOs

fbf5f23

commit

987d8d8

commit

6881f52

docs

4c7358d

commit

6af255e

commit

ad64ae8

bufdev approved these changes Dec 1, 2023

View reviewed changes

bufdev changed the title ~~Add Branch history mutation APIs~~ Revamp CreateCommits to just create Commits and add specialized Push and Rebase endpoints Dec 1, 2023

COmmit -> commit

611c669

nicksnyder reviewed Dec 1, 2023

View reviewed changes

akshayjshah reviewed Dec 1, 2023

View reviewed changes

nicksnyder reviewed Dec 1, 2023

View reviewed changes

bufdev and others added 3 commits December 1, 2023 14:17

Update branch_service.proto

23d1232

Co-authored-by: Akshay Shah <[email protected]>

Update branch_service.proto

ea3b724

Co-authored-by: Nick Snyder <[email protected]>

Update branch_service.proto

f592c6b

Co-authored-by: Nick Snyder <[email protected]>

unmultimedio reviewed Dec 1, 2023

View reviewed changes

buf/registry/module/v1beta1/branch_service.proto Outdated Show resolved Hide resolved

Update some docs

b78f4ca

saquibmian commented Dec 4, 2023

View reviewed changes

saquibmian added 2 commits December 4, 2023 12:24

base_commit_id

25c645c

Update idempotency annotations

edc5e89

nicksnyder reviewed Dec 4, 2023

View reviewed changes

buf/registry/module/v1beta1/branch_service.proto Outdated Show resolved Hide resolved

saquibmian added 2 commits December 4, 2023 15:38

typo

d6538eb

Update comments everwhere

d16c8d0

saquibmian requested a review from nicksnyder December 4, 2023 20:49

nicksnyder approved these changes Dec 4, 2023

View reviewed changes

saquibmian requested review from akshayjshah and bufdev December 4, 2023 21:04

nicksnyder merged commit f17f700 into main Dec 4, 2023
6 checks passed

nicksnyder deleted the smian/proposed-atomic-branch-history-api branch December 4, 2023 22:20

nicksnyder mentioned this pull request Dec 5, 2023

Replace the concept of "Release branch" with "Default branch" #48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp CreateCommits to just create Commits and add specialized Push and Rebase endpoints #43

Revamp CreateCommits to just create Commits and add specialized Push and Rebase endpoints #43

saquibmian commented Nov 30, 2023 •

edited

Loading

saquibmian Nov 30, 2023

bufdev commented Dec 1, 2023

akshayjshah Dec 1, 2023

saquibmian Dec 1, 2023

nicksnyder Dec 1, 2023

bufdev Dec 1, 2023

saquibmian Dec 4, 2023

nicksnyder Dec 4, 2023

saquibmian Dec 4, 2023

saquibmian Dec 4, 2023

nicksnyder Dec 4, 2023

saquibmian Dec 4, 2023

nicksnyder commented Dec 4, 2023

nicksnyder commented Dec 4, 2023

Revamp CreateCommits to just create Commits and add specialized Push and Rebase endpoints #43

Revamp CreateCommits to just create Commits and add specialized Push and Rebase endpoints #43

Conversation

saquibmian commented Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

bufdev commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicksnyder commented Dec 4, 2023

nicksnyder commented Dec 4, 2023

saquibmian commented Nov 30, 2023 •

edited

Loading