D1 export api change #6203

geelen · 2024-07-05T03:49:42Z

What this PR solves / how to test

Internal API change for D1 import/export.

Author has addressed the following

Tests
- TODO (before merge)
- Included
- Not necessary because: behaviour unchanged
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required / Maybe required
- Not required because: behaviour unchanged
Changeset (Changeset guidelines)
- TODO (before merge)
- Included
- Not necessary because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Not necessary because: covered by D1 OpenAPI description

changeset-bot · 2024-07-05T03:49:45Z

🦋 Changeset detected

Latest commit: 18b58c9

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-actions · 2024-07-05T03:51:55Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10011021497/npm-package-wrangler-6203

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6203/npm-package-wrangler-6203

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10011021497/npm-package-wrangler-6203 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10011021497/npm-package-create-cloudflare-6203 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10011021497/npm-package-cloudflare-kv-asset-handler-6203

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10011021497/npm-package-miniflare-6203

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10011021497/npm-package-cloudflare-pages-shared-6203

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10011021497/npm-package-cloudflare-vitest-pool-workers-6203

Note that these links will no longer work once the GitHub Actions artifact expires.

[email protected] includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240718.0
`workerd`	1.20240718.0	1.20240718.0
`workerd --version`	1.20240718.0	2024-07-18

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

petebacondarwin · 2024-07-05T06:06:18Z

As D1 is now GA, I assume that the API cannot be broken by changing the fields that are expected in requests and returned in responses, right? Otherwise previous Wrangler versions will now start failing. Can you confirm this is the case.

petebacondarwin · 2024-07-05T06:51:15Z

Is there a link to an internal document or ticket describing this API change?

geelen · 2024-07-08T06:56:48Z

Yeah we'll be providing backwards-compatible API request/responses for versions of Wrangler that predate this change being released (based on user-agent). Internal ticket is CFSQL-942.

petebacondarwin

In order for this to trigger a release of Wrangler (and this code) it needs a "patch" changeset.

petebacondarwin

Looks like you need to update some tests.

geelen · 2024-07-11T04:14:32Z

@petebacondarwin added changeset and passing all tests

petebacondarwin · 2024-07-11T09:53:32Z

I tried re-running the CI jobs a number of times and I am seeing lots of OOM errors.
I can't tell if that is due to changes here, a new problem with Wrangler, or just flakes.

CarmenPopoviciu · 2024-07-18T11:54:25Z

@geelen can you please rebase your changes on top of main and resolve the conflicts? Maybe that fixes the tests failure we're seeing? 🤞

.changeset/lovely-teachers-breathe.md

RamIdeas · 2024-07-18T13:23:40Z

@geelen can you please rebase your changes on top of main and resolve the conflicts? Maybe that fixes the tests failure we're seeing? 🤞

@geelen I rebased from main and resolved the conflicts on your behalf to get a head start on whether that will fix the OOM test failures

Co-authored-by: Carmen Popoviciu <[email protected]>

RamIdeas · 2024-07-19T13:01:41Z

Been looking into this OOM test failure.

Even though vitest makes it look like the "should handle local" test is failing it is actually the "should handle remote" test.

And it looks like the code is infinitely calling pollExport recursively in this else-branch https://github.com/cloudflare/workers-sdk/blob/glen/d1-export-api-change/packages/wrangler/src/d1/export.ts#L255 because response.status is "active" not "complete" or "error"

… periods

RamIdeas · 2024-07-19T16:11:18Z

packages/wrangler/src/__tests__/d1/export.test.ts

+ // This endpoint is polled recursively. If we respond immediately,
+ // the callstack builds up quickly leading to a hard-to-debug OOM error.
+ // This timeout ensures that if the endpoint is accidently polled infinitely
+ // the test will timeout before breaching available memory
+ await setTimeout(10);
+
 const body = (await request.json()) as Record<string, unknown>;

 // First request, initiates a new task
- if (!body.currentBookmark) {
+ if (!body.current_bookmark) {


This missed renaming from currentBookmark to current_bookmark was the culprit of the OOM

I have also added a timeout above to avoid this happening in the future. The OOM error was very opaque and hard to debug with many red herrings.

u da best Rahul <3!!!

CarmenPopoviciu · 2024-07-19T16:26:28Z

much awesomeness @RamIdeas 🎸 ! Thx so much for helping out here 🫶 ! @geelen this is now merged! thx both of you for making wrangler better <3

geelen requested review from a team as code owners July 5, 2024 03:49

petebacondarwin reviewed Jul 8, 2024

View reviewed changes

petebacondarwin requested changes Jul 8, 2024

View reviewed changes

geelen force-pushed the glen/d1-export-api-change branch from e56b9b6 to 6e4f51f Compare July 11, 2024 01:42

geelen force-pushed the glen/d1-export-api-change branch from 6e4f51f to 3ecee31 Compare July 11, 2024 07:03

petebacondarwin approved these changes Jul 11, 2024

View reviewed changes

CarmenPopoviciu reviewed Jul 18, 2024

View reviewed changes

.changeset/lovely-teachers-breathe.md Outdated Show resolved Hide resolved

RamIdeas force-pushed the glen/d1-export-api-change branch from 3ecee31 to 1c8e8e2 Compare July 18, 2024 11:57

geelen and others added 3 commits July 19, 2024 11:54

Adding staging header for export

2427ae8

Switched to snake_case for all D1 import/export actions

d648b3e

Update .changeset/lovely-teachers-breathe.md

557b356

Co-authored-by: Carmen Popoviciu <[email protected]>

RamIdeas force-pushed the glen/d1-export-api-change branch from 5febe40 to 557b356 Compare July 19, 2024 10:55

RamIdeas added 2 commits July 19, 2024 16:58

fix: mock export API should expect current_bookmark not currentBookmark

49b9449

add small delay in recursive poll to avoid OOM within reasonable time…

18b58c9

… periods

RamIdeas reviewed Jul 19, 2024

View reviewed changes

CarmenPopoviciu merged commit 5462ead into main Jul 19, 2024
18 checks passed

CarmenPopoviciu deleted the glen/d1-export-api-change branch July 19, 2024 16:25

workers-devprod mentioned this pull request Jul 19, 2024

Version Packages #6298

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

D1 export api change #6203

D1 export api change #6203

geelen commented Jul 5, 2024 •

edited

Loading

changeset-bot bot commented Jul 5, 2024 •

edited

Loading

github-actions bot commented Jul 5, 2024 •

edited

Loading

petebacondarwin commented Jul 5, 2024

petebacondarwin commented Jul 5, 2024

geelen commented Jul 8, 2024

petebacondarwin left a comment

petebacondarwin left a comment

geelen commented Jul 11, 2024

petebacondarwin commented Jul 11, 2024

CarmenPopoviciu commented Jul 18, 2024

RamIdeas commented Jul 18, 2024

RamIdeas commented Jul 19, 2024 •

edited

Loading

RamIdeas Jul 19, 2024

CarmenPopoviciu Jul 19, 2024

CarmenPopoviciu commented Jul 19, 2024

D1 export api change #6203

D1 export api change #6203

Conversation

geelen commented Jul 5, 2024 • edited Loading

What this PR solves / how to test

Author has addressed the following

changeset-bot bot commented Jul 5, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Jul 5, 2024 • edited Loading

petebacondarwin commented Jul 5, 2024

petebacondarwin commented Jul 5, 2024

geelen commented Jul 8, 2024

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

geelen commented Jul 11, 2024

petebacondarwin commented Jul 11, 2024

CarmenPopoviciu commented Jul 18, 2024

RamIdeas commented Jul 18, 2024

RamIdeas commented Jul 19, 2024 • edited Loading

RamIdeas Jul 19, 2024

Choose a reason for hiding this comment

CarmenPopoviciu Jul 19, 2024

Choose a reason for hiding this comment

CarmenPopoviciu commented Jul 19, 2024

geelen commented Jul 5, 2024 •

edited

Loading

changeset-bot bot commented Jul 5, 2024 •

edited

Loading

github-actions bot commented Jul 5, 2024 •

edited

Loading

RamIdeas commented Jul 19, 2024 •

edited

Loading