feat: fix keeping query plan dispatch checks through singleflight#3119
Open
barakmich wants to merge 1 commit into
Open
feat: fix keeping query plan dispatch checks through singleflight#3119barakmich wants to merge 1 commit into
barakmich wants to merge 1 commit into
Conversation
github-actions
Bot
added
area/tooling
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3119 +/- ##
==========================================
+ Coverage 75.68% 75.68% +0.01%
==========================================
Files 502 502
Lines 61989 62017 +28
==========================================
+ Hits 46909 46932 +23
- Misses 11665 11669 +4
- Partials 3415 3416 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: a5fb8e1 | Previous: 8a01e69 | Ratio |
|---|---|---|---|
BenchmarkDatastoreDriver/cockroachdb-overlap-static/TestTuple/SnapshotReverseRead (github.com/authzed/spicedb/internal/datastore/benchmark) |
43288767 ns/op 173980 B/op 20198 allocs/op |
7033740 ns/op 172723 B/op 20195 allocs/op |
6.15 |
BenchmarkDatastoreDriver/cockroachdb-overlap-static/TestTuple/SnapshotReverseRead (github.com/authzed/spicedb/internal/datastore/benchmark) - ns/op |
43288767 ns/op |
7033740 ns/op |
6.15 |
BenchmarkDatastoreDriver/cockroachdb-overlap-insecure/TestTuple/SnapshotReverseRead (github.com/authzed/spicedb/internal/datastore/benchmark) |
42906143 ns/op 172537 B/op 20195 allocs/op |
7027604 ns/op 172707 B/op 20195 allocs/op |
6.11 |
BenchmarkDatastoreDriver/cockroachdb-overlap-insecure/TestTuple/SnapshotReverseRead (github.com/authzed/spicedb/internal/datastore/benchmark) - ns/op |
42906143 ns/op |
7027604 ns/op |
6.11 |
BenchmarkDatastoreDriver/cockroachdb-overlap-insecure/TestTuple/Touch (github.com/authzed/spicedb/internal/datastore/benchmark) |
6029042 ns/op 22180 B/op 287 allocs/op |
2954634 ns/op 22021 B/op 287 allocs/op |
2.04 |
BenchmarkDatastoreDriver/cockroachdb-overlap-insecure/TestTuple/Touch (github.com/authzed/spicedb/internal/datastore/benchmark) - ns/op |
6029042 ns/op |
2954634 ns/op |
2.04 |
BenchmarkDatastoreDriver/cockroachdb-overlap-insecure/TestTuple/Create (github.com/authzed/spicedb/internal/datastore/benchmark) |
5833145 ns/op 19551 B/op 281 allocs/op |
2743304 ns/op 19496 B/op 281 allocs/op |
2.13 |
BenchmarkDatastoreDriver/cockroachdb-overlap-insecure/TestTuple/Create (github.com/authzed/spicedb/internal/datastore/benchmark) - ns/op |
5833145 ns/op |
2743304 ns/op |
2.13 |
This comment was automatically generated by workflow using github-action-benchmark.
tstirrat15
reviewed
May 14, 2026
Comment on lines
+154
to
+157
| // single-value singleflight model. Recursion safety is provided by the | ||
| // receiver-side DispatchExecutor, which refuses to dispatch any alias | ||
| // whose key is already in PlanContext.in_progress_keys, so a plan-check | ||
| // can never recurse to itself with the same dispatch key. |
Contributor
There was a problem hiding this comment.
With the potential issue here being a singleflight deadlock if this guarantee isn't present?
| assertCounterWithLabel(t, reg, 1, "spicedb_dispatch_single_flight_total", "DispatchQueryPlan") | ||
| } | ||
|
|
||
| func TestDispatchQueryPlanCheckCoalescesConcurrentCalls(t *testing.T) { |
Contributor
There was a problem hiding this comment.
If this isn't talking to a datastore, this would be a good candidate for synctest.
barakmich
force-pushed
the
barakmich/singleflight
branch
from
ed3b15d to
a5fb8e1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
We have singleflight, but it doesn't have a group for query plans. Check is the most meaningful one here (same reason we don't have a group for LR or LS) so let's make checks singleflight-able