fix(server): Use BucketSet directly in OnChange#7095
Conversation
|
augment review |
🤖 Augment PR SummarySummary: This PR refactors insert change notifications to pass a precomputed Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| void SerializerBase::OnChangeBlocking(DbIndex db_index, std::string_view key) { | ||
| void SerializerBase::OnChangeBlocking(DbIndex db_index, const PrimeTable::BucketSet& buckets) { | ||
| // We must acquire the mutex ahead and process all buckets under the same lock. | ||
| // This ensures that CVCUponInsert and the table insertion that invoked this callback |
There was a problem hiding this comment.
The comment here still says CVCUponInsert is executed in this function, but the insert path now receives a precomputed PrimeTable::BucketSet, so the wording about state alignment/linear ordering seems outdated. Consider updating the comment (and the related header comment) so it matches the new data flow.
Severity: low
Other Locations
src/server/serializer_base.h:95
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
| }; | ||
| // ChangeReq - describes the change to the table: either single bucket or whole bucket set. | ||
| using ChangeReq = std::variant<PrimeTable::bucket_iterator, PrimeTable::BucketSet>; |
There was a problem hiding this comment.
Now that ChangeReq is a std::variant<PrimeTable::bucket_iterator, PrimeTable::BucketSet>, the replication/serialization docs that describe ChangeReq::update() and req.change appear stale and could mislead future work. Consider updating docs/shard-serialization.md to reflect the new variant-based API.
Severity: low
Other Locations
docs/shard-serialization.md:169
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Pull request overview
This PR updates the DbSlice on-change notification path to compute the DashTable BucketSet once (at the mutation site) and pass it through callbacks, instead of recomputing it inside listeners. This continues the snapshot/OnChange correctness work from #7068 by reducing redundant work and avoiding passing transient insertion data.
Changes:
- Replace
ChangeReqwith astd::variant<bucket_iterator, BucketSet>to carry either a single bucket or a precomputed insertion bucket set. - Update
SerializerBaseOnChange handling to accept aBucketSetdirectly for insertion notifications. - Compute
BucketSetonce inDbSlice::AddOrFindInternaland pass it toCallChangeCallbacks; addBucketSet::operator==for invariants/debugging.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/server/table.h | Simplifies change notification payload to a variant including BucketSet. |
| src/server/serializer_base.h | Updates insertion OnChange API to take BucketSet instead of a key. |
| src/server/serializer_base.cc | Dispatches variant directly and processes passed BucketSet buckets. |
| src/server/db_slice.cc | Computes BucketSet once for insertions and updates callback consumers accordingly. |
| src/core/dash.h | Adds equality operator for BucketSet to support comparisons/assertions. |
| // Set of possible insertion buckets must be the same after possibly blocking call | ||
| DCHECK(bucket_set == db.prime.CVCUponInsert(key)); |
There was a problem hiding this comment.
The new DCHECK(bucket_set == db.prime.CVCUponInsert(key)) can be violated if CallChangeCallbacks yields and other fibers mutate the table in-between, causing CVCUponInsert to return a different BucketSet for the same key. This risks flaky DCHECK failures in debug/testing; consider removing this assertion or enforcing a real exclusion if you need this invariant.
| // Set of possible insertion buckets must be the same after possibly blocking call | |
| DCHECK(bucket_set == db.prime.CVCUponInsert(key)); | |
| // Do not re-check CVCUponInsert(key) after callbacks: they may yield and allow other fibers | |
| // to mutate the table, which can legitimately change the candidate bucket set. |
There was a problem hiding this comment.
That is true and exactly why I have it do see if it's really happens in practise
There was a problem hiding this comment.
then make it clear in the comments that if this breaks it's not a bug, but something we do not handle yet in the implementation.
|
|
||
| // It's a new entry. | ||
| CallChangeCallbacks(cntx.db_index, ChangeReq{key}); | ||
| auto bucket_set = db.prime.CVCUponInsert(key); |
There was a problem hiding this comment.
there is some performance impact as we do it now even when there are no registered callbacks.
Compute bucket set once in DbSlice and pass it on to OnChange callbacks as it is
Continuation of #7068