/go/{libraries,store}: block on lock option#10442
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes the local journaling store’s exclusive manifest lock timeout configurable, plumbing a new option from the file DB factory down into the NBS journaling manifest lock acquisition.
Changes:
- Add
ManifestLockTimeouttonbs.JournalingStoreOptionsand thread options throughnewJournalManifest. - Introduce
lockWithOptionalTimeouthelper to centralize “use default if unset” timeout behavior. - Add
journal_lock_timeout_msDBFactory param and a test that verifies a longer timeout allows waiting for lock release.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| go/store/nbs/store.go | Extends journaling store options with a configurable manifest lock timeout and passes options into journal manifest creation. |
| go/store/nbs/journal.go | Updates newJournalManifest to accept options and use the optional timeout when acquiring the lock. |
| go/store/nbs/journal_test.go | Updates tests for the newJournalManifest signature change. |
| go/store/nbs/fslock_util.go | Adds helper for acquiring fs locks with an optional (defaulted) timeout. |
| go/store/nbs/file_manifest.go | Switches file manifest lock acquisition to use the new helper (no behavioral change intended). |
| go/libraries/doltcore/dbfactory/file.go | Adds journal_lock_timeout_ms param parsing and wires it into nbs.JournalingStoreOptions. |
| go/libraries/doltcore/dbfactory/file_cache_test.go | Adds test validating that increasing the lock timeout allows a second open to wait for lock release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
coffeegoddd
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
go/store/nbs/journal.go
Outdated
| if opts.BlockOnLock { | ||
| for { | ||
| err = lock.TryLock() | ||
| if err == nil { | ||
| break | ||
| } | ||
| if !errors.Is(err, fslock.ErrLocked) { | ||
| return nil, err | ||
| } | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| case <-time.After(10 * time.Millisecond): | ||
| } |
There was a problem hiding this comment.
In the BlockOnLock loop, using time.After inside the retry loop allocates a new timer each iteration and can create unnecessary GC churn if the lock is held for a while. Prefer a single time.Ticker (defer Stop) or a sleep strategy that doesn't allocate per-iteration, and consider hoisting the retry interval into a named const for clarity/tuning.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@coffeegoddd DOLT
|
|
@coffeegoddd I've opened a new pull request, #10444, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: coffeegoddd <43383835+coffeegoddd@users.noreply.github.com>
Co-authored-by: coffeegoddd <43383835+coffeegoddd@users.noreply.github.com>
Optimize BlockOnLock retry loop to eliminate per-iteration allocations
|
@coffeegoddd DOLT
|
|
@coffeegoddd DOLT
|
No description provided.