GitBlobstore: implement Put with CAS retries + configurable identity; add Put tests#10417
GitBlobstore: implement Put with CAS retries + configurable identity; add Put tests#10417coffeegoddd merged 5 commits intomainfrom
Put with CAS retries + configurable identity; add Put tests#10417Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the Put method for GitBlobstore, enabling write operations with compare-and-swap (CAS) retry logic to handle concurrent writers. The implementation uses exponential backoff and preserves other keys when conflicts occur, making it safe for multi-writer scenarios.
Changes:
- Implemented
GitBlobstore.Putwith CAS retry logic and exponential backoff for handling concurrent updates - Added configurable identity support via
NewGitBlobstoreWithIdentityfor controlling git commit author/committer - Added comprehensive unit tests for Put including round-trip verification, overwrite behavior, and contention scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| go/store/blobstore/git_blobstore.go | Implements Put method with CAS retries, adds identity configuration, and includes helper functions for building commits and managing temporary indexes |
| go/store/blobstore/git_blobstore_test.go | Adds test infrastructure (requireGitOnPath, testIdentity) and comprehensive tests for Put including basic round-trip, overwrite, and concurrent writer scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@coffeegoddd DOLT
|
reltuk
left a comment
There was a problem hiding this comment.
Looks good. A couple comments.
| } | ||
| } | ||
|
|
||
| if err := gbs.api.UpdateIndexCacheInfo(ctx, indexFile, "100644", blobOID, key); err != nil { |
There was a problem hiding this comment.
Can you do some testing and verify whether this is idempotent? If it isn't, can we make it so?
That is to say: If the file already exists, we should succeed here (AFAIK...)
There was a problem hiding this comment.
I believe its idempotent for files (overwrites) but will error if the a directory prefix is already staged, then you try writing that. Added a test that demonstrates this, though I don't expect this to be a problem for us since we are writting files only: https://github.com/dolthub/dolt/pull/10417/changes#diff-7e67da3161a3fc914b18c9152a5ea340fbf582ca9cb2efc2dc5ef6ab7b0d5c20R335
|
@coffeegoddd DOLT
|
|
@coffeegoddd DOLT
|
|
@coffeegoddd DOLT
|
|
@coffeegoddd DOLT
|
This PR adds the first GitBlobstore write path:
GitBlobstore.Put, implemented on top of the existing internal/git.GitAPI plumbing. It also adds unit tests for Put, including a contention scenario to verify we don’t clobber concurrent writers.