-
Notifications
You must be signed in to change notification settings - Fork 280
fix: Reduce number of DB queries needed in ReverseExpand #2567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ReverseExpandQuery
participant queryDedupeMap
Caller->>ReverseExpandQuery: queryForTuples(...)
ReverseExpandQuery->>ReverseExpandQuery: isDuplicateQuery(...)
ReverseExpandQuery->>queryDedupeMap: Check/Update dedupe state
alt Not duplicate
ReverseExpandQuery->>ReverseExpandQuery: executeQueryJob(...)
end
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
9d5b4e2
to
85d91ce
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2567 +/- ##
==========================================
- Coverage 90.35% 90.33% -0.02%
==========================================
Files 140 140
Lines 19583 19589 +6
==========================================
Hits 17693 17693
- Misses 1440 1444 +4
- Partials 450 452 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could achieve similar results by using golang.org/x/sync/singleflight
; see
openfga/pkg/typesystem/resolver.go
Line 35 in e147e4b
lookupGroup := singleflight.Group{} |
Description
What problem is being solved?
In the weighted reverse_expand, each individual leaf node kicks off its own independent chain of queries. Each query chain attempts to deduplicate its own work, but for some models these query chains can start to overlap as they move up the graph because they have common ancestors. This model for example, has 3 leaf nodes which all share a common ancestor in
document#viewer
:You can clearly see the common ancestor

document#viewer
in the graph:If a
user
is related todocument:a
via#commenter
,#editor
, and#writer
, all 3 of those independent branches will attempt to read from the DB to find the common ancestordocument#owner@document:a#viewer
. That means this query will run 3 times instead of 1.How is it being solved?
We have an existing
jobDedupeMap
, but it is scoped within each query chain. So multiple query chains which lead to the same tuples will do duplicate work.What changes are made to solve it?
Add
queryDedupeMap
to theReverseExpandQuery
object itself, so all querying routines will be aware of all duplicate work.References
Review Checklist
main
Summary by CodeRabbit