fix: batch sites uploads in groups under 100mb #1195
Conversation
There's an upper limit on the size of an upload to the bulk kv put api (as specified in https://api.cloudflare.com/#workers-kv-namespace-write-multiple-key-value-pairs). This patch batches sites uploads staying under the 100mb limit. Fixes #1187
🦋 Changeset detectedLatest commit: 7147ff7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2489229231/npm-package-wrangler-1195You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/1195/npm-package-wrangler-1195Or you can use npx https://prerelease-registry.developers.workers.dev/runs/2489229231/npm-package-wrangler-1195 dev path/to/script.js |
|
I tried the first test version the bot linked and it doesn't appear to have fixed the issue.🤔 |
|
Oh for real? I thought I'd extensively test this tomorrow, but good to know it already doesn't work lol. |
|
Yup, still the same 413 error. |
rozenmd
dismissed
their stale review
Didn't see that the issue wasn't fixed - apologies
threepointone
changed the title
|
@threepointone Any workaround for this? Need to update my full site soon and this is still blocking. If not then I will probably have to downgrade to Wranger v1 for the time being. |
|
We want to land this next week! Hopefully earlier than later. |
|
You should feel free to downgrade if it's blocking you tho, terribly sorry for the inconvenience :( |
rozenmd
changed the title
|
Hey @EatonZ, do you mind retrying with the latest test version in the comment above? ( |
| // Since the bulk upload api endpoint stays the same | ||
| // We're going to have to clear the mock as soon as it's resolved | ||
| // And immediately add a mock for another one | ||
| // Welcome to a callback pyramid in 2022 |
There was a problem hiding this comment.
I think we could make this a bit less painful if we capture useful stuff in the handler and then check it after runWrangler has completed...
const requests = mockUploadAssetsToKVRequest(kvNamespace.id);
await runWrangler("publish);
expect(requests.uploads).toEqual(expectedAssets);
Either write out the expectedAssets matchers by hand, or generate them based on the assets array.
We would need to tweak the mockUploadAssetsToKVRequest() so that you don't need to specify assets as a parameter, and instead add them to a requests object that should be returned from the call.
There was a problem hiding this comment.
I've added a follow-up task for this as it'll require refactoring the other tests too: #1245
packages/wrangler/src/sites.tsx
Outdated
| // delete all the unused assets | ||
| deleteKVBulkKeyValue(accountId, namespace, Array.from(namespaceKeys)), | ||
| ]); | ||
| // sequentially upload each bucket |
There was a problem hiding this comment.
FWIW @threepointone @petebacondarwin: I compared uploading a 150MiB site via this sequential solution, and a Promise.all approach:
const bucketsToPut = [];
for (const bucket of uploadBuckets) {
bucketsToPut.push(putKVBulkKeyValue(accountId, namespace, bucket));
}
await Promise.all(bucketsToPut);
The sequential solution took 51 sec while the parallel solution took 38 sec, could be worth looking into if folks complain it's too slow
There was a problem hiding this comment.
I can't recollect why I made this sequential, probably to avoid saturating the network. We could make this parallel, and consider adding limited concurrency later if people complain with extremely large payloads (like, GBs)
|
I can't stamp this because I started the PR 😅 It's other wise LGTM, feel free to take a call on the recommended changes (or file issues to do them in subsequent PRs) |
|
Extremely grateful to you @rozenmd!!! Thanks so very much. |
|
@rozenmd I tested the 2.0.11 release and it works for me now! |
There's an upper limit on the size of an upload to the bulk kv put api (as specified in https://api.cloudflare.com/#workers-kv-namespace-write-multiple-key-value-pairs). This patch batches sites uploads staying under the 100mb limit, after base64 encoding the files.
Fixes #1187
This fix could be improved by moving it to
kv.ts:putKVBulkKeyValue()but I couldn't figure out a clean solution. So once this lands, I'll file an issue and make a followup PR. We'll probably need it for people who usewrangler kv:bulk putanyway.