sync: pre-build entitlement-id set in processGrantsWithExternalPrincipals#851
sync: pre-build entitlement-id set in processGrantsWithExternalPrincipals#851btipling wants to merge 1 commit into
Conversation
…pals Per-grant inner-loop scan over entitlements was O(N) per grant; total O(M*N) where M = grants, N = entitlements per resource. C1 uplift hits this path on every external-principal grant during bucketing, which at the tenant scale represented in c1-uplift-perf-r1.md (millions of grants per Klaviyo / Lilly-class sync) accounts for material wall-clock during post-sync uplift. Pre-build the entitlement-id set once per resource. Inner check becomes O(1). Total O(M + N) per resource. No interface changes, no behavior change for any grant; same iteration order, same side effects. Existing TestExternalResource* coverage continues to pass. Driven by c1 uplift perf campaign tracker candidate (see investigations/c1-uplift-perf-tracker.md in stargate). Sibling change to PR #849 (compareProto -> proto.Equal) in the same campaign.
General PR Review: sync: pre-build entitlement-id set in processGrantsWithExternalPrincipalsBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThis PR replaces per-slug Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // existence checks below. Entitlement counts are 10^2-10^4 on real | ||
| // tenants, so the in-memory set fits comfortably and the grant-iteration | ||
| // loop's inner GetEntitlement fanout collapses to set membership. | ||
| knownEntitlementIDs, err := s.listAllEntitlementIDs(ctx) |
There was a problem hiding this comment.
- Do you know the biggest entitlement count, currently? Some connectors have dynamic ents.
- We do this before we know that we need it. We could make it lazy (use a once or something). IF the answer to 1 is actually 10^4, I don't care anymore.
kans
left a comment
kans
left a comment
There was a problem hiding this comment.
Address comment before merge.
|
LGTM but I'm scared of not knowing enough about uplift and lifecycle |
3 participants
Summary
C1 uplift perf campaign — second baton-sdk PR (sibling to #849).
processGrantsWithExternalPrincipalsissues as.store.GetEntitlementper slug per principal-match while iterating every external-source grant viaListWithAnnotations. The pattern repeats in both theMatchIDandMatchExternalbranches. At tenant scales the campaign targets (millions of grants per sync, large entitlement counts per resource), this is material wall-clock during c1's post-sync uplift.This pre-builds the entitlement-ID set once per call via one paginated
ListEntitlements. Inner-loop existence check becomes O(1) set membership. No behavior change.What changes
pkg/sync/syncer.go: NewlistAllEntitlementIDshelper paginatesListEntitlementsinto amapset.Set[string](existing project idiom — see L1901, L2486).processGrantsWithExternalPrincipalsbuilds the set once at function entry and replaces both per-slugGetEntitlementexistence checks withknownEntitlementIDs.ContainsOne(newExpandableEntId). The existing "found no entitlement with entitlement id generated from external source sync" error log is preserved; the surroundingsql.ErrNoRowsplumbing is no longer needed.Perf rationale
GetEntitlementper (grant × matched slug)GetEntitlementListEntitlementsFor a tenant with 10^5 external-source grants and an average match/slug fanout of 5, this collapses ~500k per-row
GetEntitlementqueries into a single paginated list.No benchmark currently covers
processGrantsWithExternalPrincipals; before/after numbers are not included.Backwards compatibility
Function signature unchanged. No new params, no new return values, no interface changes. All existing callers continue to compile and behave identically.
The single new helper (
listAllEntitlementIDs) is private to the package.Self-review
+40/-14lines, one file, no interface changes.mapset.Set[string]fromgithub.com/deckarep/golang-set/v2) matches the existing idiom in the same file.TestExternalResource*coverage (TestExternalResourceMatchID,TestExternalResourceMatchIDWithExpandableRemapping,TestExternalResourceEmailMatch,TestExternalResourceGroupProfileMatch,TestExternalResourceWithGrantToEntitlement) all pass.gofmtclean.Self-review clean.
Test plan
go build ./...go test -count=1 ./pkg/sync/ -v— all pass, includingTestExternalResource*go test ./...— all passRelated
investigations/c1-uplift-perf-tracker.md