Skip to content

Conversation

@arreyder
Copy link
Contributor

@arreyder arreyder commented Dec 23, 2025

cache getFinishedSync result in C1File to eliminate N+1 query pattern during pagination
adds sync.Once-guarded cache fields to C1File struct
replaces repeated getFinishedSync calls in listConnectorObjects with cached lookup

production CPU profiling of temporal_sync revealed that getFinishedSync consumed 44.32s (43.11% of CPU) during sync operations. The function was being called on every listConnectorObjects pagination call when reqSyncID is not explicitly set.

ListGrants → listConnectorObjects → getFinishedSync (repeated per page)

I tested this one by reducing the workload page size so I could emulate prod a little better with my tool and I saw good results.

for this case, baton-sdk benchmarks won't show any benefit because they do not work like temporal-sync does when running in prod.

Summary by CodeRabbit

  • Chores
    • Improved performance by optimizing how sync data is retrieved during listing operations, reducing unnecessary database queries.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 23, 2025

Walkthrough

This pull request adds internal caching to C1File to optimize sync run retrieval and avoid N+1 queries. Three unexported cache fields are introduced alongside a new private method getCachedViewSyncRun that uses sync.Once to cache sync run results. The listConnectorObjects function is updated to use this cached sync instead of directly fetching the latest sync.

Changes

Cohort / File(s) Change Summary
Cache field initialization
pkg/dotc1z/c1file.go
Adds three unexported fields (cachedViewSyncRun, cachedViewSyncOnce, cachedViewSyncErr) to C1File for storing cached sync run results.
Cached sync run method
pkg/dotc1z/sync_runs.go
Implements new private method getCachedViewSyncRun(ctx context.Context) that uses sync.Once pattern to cache sync run retrieval, attempting finished full sync first, then falling back to latest unfinished sync.
Query optimization
pkg/dotc1z/sql_helpers.go
Updates listConnectorObjects to use getCachedViewSyncRun(ctx) instead of inline sync retrieval, removing the unfinished-sync fallback logic previously present.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • loganintech

Poem

🐰 A cache so fine, a sync run neat,
No more N+1, what a treat!
Once locks the path, so smooth and fleet,
With fallback grace, the loop's complete! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main objective of the PR: adding caching to eliminate N+1 queries on getFinishedSync during pagination.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch crr/cache-view-sync-run

Comment @coderabbitai help to get the list of available commands and usage tips.

ctx, span := tracer.Start(ctx, "C1File.getCachedViewSyncRun")
defer span.End()

c.cachedViewSyncOnce.Do(func() {
Copy link
Contributor

@ennyjfrick ennyjfrick Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use sync.OnceValue here instead maybe

@arreyder arreyder merged commit d79dcb4 into main Dec 23, 2025
6 checks passed
@arreyder arreyder deleted the crr/cache-view-sync-run branch December 23, 2025 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants