Page datastore queries that fetched all applications - #7234
Conversation
was noticing the ListApplications call just pulling every row in the collection for large projects. switched both piped_api and web_api to cursor-based paging. same fix for ListEvents. Co-authored-by: areebahmeddd <areebahmeddd@users.noreply.github.com> Signed-off-by: srinivasr <sriniv4sreddy@gmail.com>
✅ Deploy Preview for pipecd-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR addresses unbounded datastore reads in PipeCD’s control-plane gRPC APIs by switching several “list everything” queries to internal cursor-based pagination, aggregating results server-side to preserve existing RPC contracts while avoiding single large queries (notably improving piped sync behavior for large projects).
Changes:
- Page and aggregate results for
ListApplications(piped + web) andCreateDeploymentChainapplication lookups, using stable ordering and a fixed per-page limit. - Page and aggregate
ListEvents, including a default stable ordering when the request does not specify one. - Add Firestore composite indexes required by the new query shapes and update index parsing tests; add a unit test for paged
ListApplications.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/app/server/grpcapi/web_api.go | Switch WebAPI ListApplications to internally page via listAllApplications with a fixed limit. |
| pkg/app/server/grpcapi/piped_api.go | Add paging + stable ordering for piped ListApplications, ListEvents, and application listing used by CreateDeploymentChain. |
| pkg/app/server/grpcapi/list_applications_test.go | Add unit tests verifying multi-page aggregation behavior for piped ListApplications. |
| pkg/app/server/grpcapi/application_lister.go | Introduce a shared helper to page/aggregate application lists via datastore cursor. |
| pkg/app/server/grpcapi/event_lister.go | Introduce a shared helper to page/aggregate event lists via datastore cursor. |
| pkg/app/ops/firestoreindexensurer/indexes.json | Add composite indexes for the new ordered/paged Firestore query patterns. |
| pkg/app/ops/firestoreindexensurer/indexes_test.go | Update expected parsed index list to include the new indexes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
cb748e5 to
0ea89f0
Compare
|
this probably needs discussion before merge 🤔
the description says could the index changes land in a separate, earlier pr so operators can let them build before the query changes ship? that would avoid a hard upgrade ordering constraint
the indexes that seem to match the actual query shapes would be : could you walk through the query-shape to index mapping again? the two event indexes look correct, so this may just be a slip on the application side |
| // Page through the datastore so a large project cannot be served by one | ||
| // unbounded query. The RPC response has no cursor field, so all pages are | ||
| // aggregated here before returning. | ||
| apps, err := listAllApplications(ctx, a.applicationStore, opts) |
There was a problem hiding this comment.
ListApplications is called by every piped on each app-store sync and again by the event watcher. with 3,000 apps, this change turns one query into 30 sequential round-trips per sync, while the response is still fully aggregated in memory.
the description also says paging avoids paying for all applications on every sync, but the total read is still the same, just split across queries.
could listApplicationsPageSize be increased substantially, say to 1000, to keep the bounded-query benefit without adding 10x the round-trips? 🤔
There was a problem hiding this comment.
makes sense, bumped listApplicationsPageSize to 1000 to keep the round-trips low while still keeping each individual query bounded.
data-only and test-only change. pulling the firestore composite indexes out from pipe-cd#7234 into a prerequisite PR based on reviewer feedback. CreateIndexes in the ops component fires off gcloud commands but doesn't wait for the READY state. landing this first lets the indexes build in the background across environments before we ship the actual query changes. avoids the FAILED_PRECONDITION downtime window for ListApplications and CreateDeploymentChain. Which issue(s) this PR fixes: split from pipe-cd#7234; enables pipe-cd#7051 Co-authored-by: areebahmeddd <areebahmeddd@users.noreply.github.com> Signed-off-by: srinivasr <sriniv4sreddy@gmail.com>
0ea89f0 to
9749c8a
Compare
reviewer caught it: CreateDeploymentChain always filters on Disabled so the composite indexes from the previous commit won't actually serve those queries. swapped out the four bad entries for the three shapes that match: ProjectId | Disabled | Name | Id ProjectId | Disabled | Kind | Id ProjectId | Disabled | Name | Kind | Id the standalone ProjectId | Id one is redundant with ProjectId | Disabled | Id so dropped it too. Co-authored-by: areebahmeddd <areebahmeddd@users.noreply.github.com> Signed-off-by: srinivasr <sriniv4sreddy@gmail.com>
9749c8a to
c7dbb1a
Compare
makes sense, split the indexes out into #7274 so operators can let them build first without hitting also fixed the app index shapes to include |
Signed-off-by: srinivasr <sriniv4sreddy@gmail.com>
ea95d28 to
201421d
Compare
What this PR does:
Fixes unbounded datastore queries in
ListApplications(piped and web) andCreateDeploymentChain. These endpoints ran one query with no limit and returned every matching row; now they fetch pages of 100 and combine them before responding. Cursor paging needs a stable sort, so these queries order byId.ListEventsgets the same treatment, with a default of newest-first when a request leaves the order unset — without that default, paging would error on the second page.Adds the composite indexes these query shapes need to
firestoreindexensurer.Why we need it:
A project with thousands of applications pays for all of them on every piped sync. The
TODOinListApplicationsflagged this; until the reads are chunked, the query grows with the project and eventually risks datastore timeouts.Which issue(s) this PR fixes:
Fixes #7051
Does this PR introduce a user-facing change?:
No breaking changes. Responses still contain the complete aggregated list — they're just fetched from the backend in chunks.
firestoreindexensurercreates the new composite indexes automatically on control-plane startup, but GCP index builds take time — calls fail withFAILED_PRECONDITIONuntil each index finishes building. MySQL users need no action.