Skip to content

Cap the number of errors and warnings for bulk KV put#9775

Merged
vicb merged 2 commits intomainfrom
vicb/kv-batch-logs
Jul 2, 2025
Merged

Cap the number of errors and warnings for bulk KV put#9775
vicb merged 2 commits intomainfrom
vicb/kv-batch-logs

Conversation

@vicb
Copy link
Copy Markdown
Contributor

@vicb vicb commented Jun 27, 2025

Fixes #9498

Queueing a lot of errors/warnings can fill up all the memory and slow down the process as described in the linked issue.


@vicb vicb requested a review from a team as a code owner June 27, 2025 07:55
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 27, 2025

🦋 Changeset detected

Latest commit: 4af7085

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
wrangler Patch
@cloudflare/vitest-pool-workers Patch

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

@github-project-automation github-project-automation Bot moved this to Untriaged in workers-sdk Jun 27, 2025
@vicb vicb force-pushed the vicb/kv-batch-logs branch from 45e8c4d to a412af6 Compare June 27, 2025 07:56
@vicb vicb changed the title Cap the number od errors and warnings for bulk KV put Cap the number of errors and warnings for bulk KV put Jun 27, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 27, 2025

These changes have been automatically backported to Wrangler v3 🎉 You can view the automatically updated PR at v3-maintenance...v3-backport-9775. Please check that PR for correctness, and make sure it's merged after this one. Thank you for helping us keep Wrangler v3 supported!

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Jun 27, 2025

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@9775

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@9775

miniflare

npm i https://pkg.pr.new/miniflare@9775

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@9775

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@9775

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@9775

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@9775

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@9775

wrangler

npm i https://pkg.pr.new/wrangler@9775

commit: 4af7085

Comment thread .changeset/shaggy-frogs-tap.md Outdated
Comment thread packages/wrangler/src/kv/helpers.ts Outdated
errors.push(`The item at index ${i} is ${JSON.stringify(keyValue)}`);
if (!isKVKeyValue(keyValue) && !maxNumberOfErrorsReached) {
if (errors.length === BATCH_MAX_ERRORS_WARNINGS) {
maxNumberOfErrorsReached = true;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

instead of using this maxNumberOfErrorsReached flag, have you considered just breaking and exiting the loop? I feel like it might make sense to just quite after the few initial errors? is there any benefit in keeping looping through all the remaining entries?

Copy link
Copy Markdown
Contributor Author

@vicb vicb Jun 30, 2025

Choose a reason for hiding this comment

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

That's true, we could break if both maxNumberOfErrorsReached and maxNumberOfWarningsReached are reached. But it's probably not worth adding more code to optimize an error case?

@github-project-automation github-project-automation Bot moved this from Untriaged to Approved in workers-sdk Jun 30, 2025
Comment thread .changeset/shaggy-frogs-tap.md Outdated
@vicb vicb force-pushed the vicb/kv-batch-logs branch 2 times, most recently from a24e020 to d47cbd9 Compare July 1, 2025 07:16
Co-authored-by: Dario Piotrowicz <dario@cloudflare.com>
@vicb vicb force-pushed the vicb/kv-batch-logs branch from d47cbd9 to eee1451 Compare July 1, 2025 07:18
@vicb vicb requested a review from a team as a code owner July 1, 2025 15:03
@vicb vicb added this pull request to the merge queue Jul 2, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 2, 2025
@vicb vicb added this pull request to the merge queue Jul 2, 2025
Merged via the queue into main with commit 4309bb3 Jul 2, 2025
44 of 51 checks passed
@vicb vicb deleted the vicb/kv-batch-logs branch July 2, 2025 07:47
@github-project-automation github-project-automation Bot moved this from Approved to Done in workers-sdk Jul 2, 2025
@workers-devprod workers-devprod mentioned this pull request Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Bulk putting big JSON file with malformed records burns the CPU

3 participants