feat(app-deployments): improve app:create performance with v2 format#7565
feat(app-deployments): improve app:create performance with v2 format#7565adambenhassen wants to merge 24 commits intomainfrom
Conversation
Summary of ChangesHello @adambenhassen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant performance improvement for app deployments by implementing a new 'v2' storage format. This new format allows for efficient delta uploads, where only new or changed documents are uploaded, and enables cross-version deduplication, reducing storage and transfer overhead. The changes span across the CLI, API, and CDN worker, ensuring a seamless and faster deployment experience, especially for applications with many documents and frequent updates. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new V2 storage format for app deployments, significantly improving performance through delta uploads and parallel processing. The changes include new CLI flags for format selection and timing, robust SHA256 hash validation, and optimized parallel uploads to S3 and ClickHouse. Extensive integration tests have been added to cover the new functionality, and a benchmark script with a detailed README provides clear performance comparisons. The changes are well-implemented and effectively address the stated performance goals. There is a minor style guide adherence opportunity regarding environment variable access in a utility script and a backend service provider.
adambenhassen
force-pushed
the
adam/improve-app-deployment-perf
branch
from
d1672f8 to
2db60d1
Compare
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-hive/cli |
0.58.0-alpha-20260128114751-69c3012b7a3f4155c12d74c957774ed07b375362 |
npm ↗︎ unpkg ↗︎ |
hive |
9.3.0-alpha-20260128114751-69c3012b7a3f4155c12d74c957774ed07b375362 |
npm ↗︎ unpkg ↗︎ |
📚 Storybook DeploymentThe latest changes are available as preview in: https://pr-7565.hive-storybook.pages.dev |
|
🐋 This PR was built and pushed to the following Docker images: Targets: Platforms: Image Tag: |
💻 Website PreviewThe latest changes are available as preview in: https://pr-7565.hive-landing-page.pages.dev |
adambenhassen
force-pushed
the
adam/improve-app-deployment-perf
branch
from
133d445 to
69c3012
Compare
n1ru4l
left a comment
n1ru4l
left a comment
There was a problem hiding this comment.
I added a few comments on the functionality, happy to discuss these a more if needed.
The main issue for me is the fact that operations are no longer scoped to the app version. I proposed a potential solution to the problem.
Aside from that, the user experience from the CLI standpoint could be improved, as it is desired that using sha256 hashed would become the default way people upload persisted documents.
| const response = await this.request({ | ||
| key, | ||
| // Try v2 key first | ||
| const keyV2 = buildOperationS3BucketKeyV2(targetId, appName, hash); |
There was a problem hiding this comment.
Instead of trying v2 first and then fallback back to v1, maybe we encode the format within the is app deployment active key to avoid the extra roundtrip?
There was a problem hiding this comment.
Yeah, that sounds good. let's have that.
Update: The only problem is that apps-enabled is only written during activation, and at that point we don't know the document format. The format is only known during addDocumentToAppDeployment.
I propose the following approach:
- Upload: write
v1-inactiveorv2-inactivetoapps-enabled - Activation: overwrite with
v1orv2(read existing value, strip-inactive, write back) - CDN:
HEAD->GET, read body. If it contains-inactiveor is0-> not active. Ifv1/v2/1-> active, use format accordingly.
Backward compat: existing deployments have 1 -> treated as active v1.
There was a problem hiding this comment.
Check my proposed changes above with the latest changes: 5cc6655
There was a problem hiding this comment.
For a better user experience, we could auto-detect whether the hashes are sha256 and then automatically use the new format for uploading. If the format is not identified as sha256 we could print some information about this (e.g. mention the speed improvements and pointing to our documentation that shows how to configure relay or codegen).
We can still keep the format parameter, so the user could enforce the old format if they need to, just in case.
There was a problem hiding this comment.
I agree, this automates it. Also requires some changes to automatically resolve target as well, but it's worth it. Implementing.
adambenhassen
force-pushed
the
adam/improve-app-deployment-perf
branch
from
8991e50 to
dd7a95a
Compare
|
Pending conflict resolution once all conversations are resolved. |
n1ru4l
left a comment
n1ru4l
left a comment
There was a problem hiding this comment.
I added some more comments on the implementation and gave some suggestions.
Aside from that I also noticed that skipped uploads documents are not visible via the Hive Console UI since we skip the Clickhouse insert, which would also kind of lead of versions with re-used documents not showing up in the conditional breaking change detection? 😅 Do I understand that correct?
Seems like it might make sense to do some more architectural decissions and eventual database adjustments or maybe still not refrain from inserting the documents into clickhouse (and sorely skip the s3 put request) or re-insert from clickhouse to clickhouse 0 not sure about that?
Maybe we should step away on the code iteration on the PR for a moment have a bit of a more high-level RFC document, that makes it faster to iterate on while thinking about all involved components? Digging through the code to understand the implementation details and discover flaws is quite time consuming for me.
| @@ -67,6 +67,7 @@ export class AppDeploymentsManager { | |||
| name: string; | |||
| version: string; | |||
| }; | |||
| hashes?: readonly string[] | null; | |||
There was a problem hiding this comment.
Should there be an assertion that makes the hashes property mandatory for the v2 format?
There was a problem hiding this comment.
No separate format field exists on CreateAppDeploymentInput; hashes presence is the v2 signal. Without hashes, the code skips v2 writes and returns empty existingHashes, safely degrading to v1. The CLI enforces this: v2 always sends hashes, v1 sends undefined. Should I add an explicit format field to make the contract stricter?
EDIT: Added.
| [this.s3[0].endpoint, this.s3[0].bucket, manifestKey].join('/'), | ||
| { method: 'HEAD', aws: { signQuery: true } }, | ||
| ); | ||
| if (existingManifest.statusCode !== 200) { |
There was a problem hiding this comment.
Why wouldn't there be an existing manifest at that stage? It should have been written there during app create according to the logic?
If we then create it only based on clickhouse - it would not contain all hashes so produce an invalid manifest?
So my question is, why do we have this logic here for falling back in the first place?
There was a problem hiding this comment.
Ok, with the format field now, from comment above, required on CreateAppDeploymentInput, v2 deployments always provide hashes at create time, so the manifest is always written during createAppDeployment. I'll remove the fallback and throw if the manifest is missing for a v2 activation, that surfaces the bug instead of silently producing an invalid manifest.
| input CreateAppDeploymentInput { | ||
| target: TargetReferenceInput | ||
| appName: String! | ||
| appVersion: String! | ||
| """ | ||
| Optional list of document hashes the client intends to upload. | ||
| If provided, the response will include the subset that already exist on the server, | ||
| enabling delta uploads (skip uploading documents that already exist). | ||
| """ | ||
| hashes: [String!] | ||
| } |
There was a problem hiding this comment.
The way I understand the implementation providing hashes indicates that this will be v2 (sha256) app deployment version. In that case, should this be documented more explicitly here?
There was a problem hiding this comment.
Addressed: added an explicit format field to CreateAppDeploymentInput
| """ | ||
| Given a list of document hashes, returns the subset that already exist for this app (across all versions). | ||
| Used for delta uploads - CLI can skip uploading documents that already exist. | ||
| """ | ||
| appDeploymentDocumentHashes(appName: String!, hashes: [String!]!): [String!]! |
There was a problem hiding this comment.
Is this still needed if it there is now CreateAppDeploymentOk.existingHashes?
There was a problem hiding this comment.
It's redundant now indeed. removing.
| """ | ||
| Fields available for sorting app deployments. | ||
| """ | ||
| enum AppDeploymentsSortField { | ||
| CREATED_AT | ||
| ACTIVATED_AT | ||
| LAST_USED | ||
| } | ||
|
|
||
| """ | ||
| Sort configuration for app deployments. | ||
| Storage format for app deployment documents. | ||
| """ | ||
| input AppDeploymentsSortInput { | ||
| field: AppDeploymentsSortField! | ||
| direction: SortDirectionType! | ||
| enum AppDeploymentFormatType { | ||
| """ | ||
| V1 format: version-scoped storage. Each version stores documents separately. | ||
| Allows any hash format. No cross-version deduplication. | ||
| """ | ||
| V1 | ||
| """ | ||
| V2 format: content-addressed storage with SHA256 hashes. | ||
| Enables cross-version deduplication and delta uploads. | ||
| """ | ||
| V2 | ||
| } |
There was a problem hiding this comment.
NIT: Should we use more descriptive naming like CUSTOM/ARBITRARY, SHA256 instead of V1 and V2?
There was a problem hiding this comment.
Agreed, it makes more sense indeed
| * Check the format of existing documents for a deployment by sampling one hash. | ||
| * Returns 'v1', 'v2', or null if no documents exist. | ||
| */ | ||
| async getDeploymentDocumentFormat(appDeploymentId: string): Promise<'v1' | 'v2' | null> { |
There was a problem hiding this comment.
Why not store the app deployment version format (whether it is using arbitrary hashes or sha256 hashes) on the pg record (app_deployments table) when initially created via Mutation.createAppDeployment?
That way on both createAppDeployment.addDocumentToAppDeployment and createAppDeployment.activeAppDeployments we avoid the roundtrip to s3 or clickhouse in order to compute the format.
There was a problem hiding this comment.
Ok. I tended to avoid changes involving postgres. Implementing.
| if ( | ||
| false === | ||
| (await deps.isAppDeploymentActive(params.targetId, params.appName, params.appVersion)) | ||
| ) { | ||
| const deploymentStatus = await deps.getAppDeploymentStatus( | ||
| params.targetId, | ||
| params.appName, | ||
| params.appVersion, | ||
| ); | ||
|
|
||
| if (!deploymentStatus.enabled) { |
There was a problem hiding this comment.
The apps-enabled key can be repurposed to store more information at the same time:
- whether the app is enabled
- what format it contains (arbitary hashes or sha256 hashes)
- which documents belong to this app version
That means for legacy versions created before these adjustments it would always contain "0" or "1" (or be non-existent). For new ones it could be a JSON object that contains the above information e.g.
{
"isActive": true,
// arbitrary/custom or sha256
"hashFormat": "sha256",
"documents": [
"hash1",
"hash2",
"hash3",
// ....
]
}This information could then be sufficient for the CDN handler to figure out on how to resolve the documents and it would reduce the need for one more S3 read.
Furthermore, it can act as a foundation for other parallel ongoing efforts.
There was a problem hiding this comment.
Agreed, but out of scope for this PR. Merging status + format + hashes into one S3 key is a separate refactor.
| const existingManifest = await this.s3[0].client.fetch( | ||
| [this.s3[0].endpoint, this.s3[0].bucket, manifestKey].join('/'), | ||
| { method: 'HEAD', aws: { signQuery: true } }, | ||
| ); |
There was a problem hiding this comment.
how do we guarantee that all documents that were specified in the manifest were uploaded/are available?
There was a problem hiding this comment.
We don't currently verify that. The manifest controls which hashes are allowed for version isolation, but if a document wasn't uploaded, the CDN returns 404 naturally. Adding an activation-time check (compare manifest hashes against uploaded documents) would catch incomplete deployments, but that's a separate concern from this PR.
Correct. in custom format, every document goes through Proposal: after
I'd rather if you continue reviewing this PR to the completion of its scope. the remaining items are additive improvements, not flaws. The core functionality is covered by extensive tests and the review hasn't surfaced any critical issues. The clickhouse copy for deduped documents and the merged S3 key (status + format + hashes) can be follow-ups without blocking what's already working. |
2 participants
Background
App deployment uploads can be slow when deploying many documents, especially when most documents haven't changed between versions. Currently, every document is re-uploaded on each deployment, even if it already exists from a previous version.
Description
Implements a new v2 storage format for app deployments that enables delta uploads: only uploading documents that don't already exist. This dramatically improves deployment speed for incremental updates.
Notable Changes
CLI:
--formatflag needed). Logs a message pointing to docs if hashes aren't SHA256.--formatflag is still available as an explicit override.createAppDeployment: existing hashes are returned in the mutation response, no separate query needed. Single round-trip.--targetis no longer required for delta optimization (server resolves from access token).--showTimingflag to display per-batch timing breakdown (requires server-sideFEATURE_FLAGS_APP_DEPLOYMENT_TIMINGS_ENABLED=1).Server:
createAppDeploymentaccepts optionalhashesinput and returnsexistingHashes: enables delta uploads in a single round-trip.appDeploymentDocumentHashesquery onTargettype acceptshashesinput and returns only the matching subset (server-side filtering via ClickHouse).cSql.longArrayfor safe handling of large hash arrays.app:create.label/durationpairs, gated behindFEATURE_FLAGS_APP_DEPLOYMENT_TIMINGS_ENABLEDenv var + clientshowTimingsopt-in.S3_UPLOAD_CONCURRENCYtoAPP_DEPLOYMENT_S3_UPLOAD_CONCURRENCY.createAppDeployment(handles 100% and partial dedup)CDN:
apps-enabledS3 key body (v1-inactive/v2-inactiveduring upload,v1/v2after activation). CDN reads the format via GET (previously HEAD) and resolves documents directly: no more v2-then-v1 fallback.app-v2-manifest/{targetId}/{appName}/{appVersion}) for version isolation. CDN fetches document and manifest in parallel, rejects hashes not belonging to the requested version.apps-enabledactive flag to prevent a race condition.Backwards Compatibility
apps-enabledbody'1'are treated as active v1.Security
Limitations
app_deploymentstable only containing activated deployments, so basically we can’t know whichapp_deployment_idvalues belong to certain target/appName for cross-version pending deployments. requires postgres data joining.Benchmarks
All changes were benchmarked with the script provided in
load-tests/app-deployments.1000 Documents
Format Comparison
app/{target}/{name}/{version}/{hash}app-v2/{target}/{name}/{hash}Integration tests added
Manual e2e tests ran
CLI:
CDN:
To summarize what was tested:
Closes CONSOLE-1582