Read concurrency#437
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
As discussed with Nate and Luke, I will be redesigning or removing the peer cancellation logic. Don't bother with review for now. |
Alrighttt
force-pushed
the
matt/read-concurrency
branch
from
eaf7c4c to
db7a3a3
Compare
|
I removed all code related to the peer cancellation mechanism. |
| // calls. Buckets obtained via the DB inside fn must not be used | ||
| // after fn returns. Writes via the DB inside fn are not durable | ||
| // and may panic. | ||
| View(fn func(DB) error) error |
There was a problem hiding this comment.
this makes me sad
maybe we could add a writeable flag on Bucket() instead?
There was a problem hiding this comment.
Not sure you'd like that more but if you want to be able to do multiple operations within a single View we could also do something like
snap := m.store.Snapshot()
defer snap.Close()
for range 10 {
_, _ = snap.Block(...)
}Where Snapshot starts a readonly transaction which returns a ReadonlyDB interface only containing a subset of the DB interface. So it's impossible to call CreateBucket in the first place.
There was a problem hiding this comment.
Implemented Chris's suggestion
There was a problem hiding this comment.
I do like the idea of making it impossible to call mutating methods, but to make it airtight, we need a read-only DBBucket as well that has no Put or Delete methods.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| default: patch | |||
There was a problem hiding this comment.
This should probably be minor considering we're extending the exported Store and DB interfaces? Anyone implementing those will have to add Snapshot for it to compile.
| } | ||
| } | ||
|
|
||
| func runConcurrentReadsWrites(t *testing.T, db chain.DB) { |
There was a problem hiding this comment.
Should we add a reader that calls AddV2PoolTransactions alongside the existing readers? Isn't that the most interesting one because we temporarily release the lock there to notify the listeners?
chris124567
left a comment
chris124567
left a comment
There was a problem hiding this comment.
LGTM besides PJ's comments
6 participants
Changes
Manager.mutosync.RWMutexto enable reads to run in parallel.