-
Notifications
You must be signed in to change notification settings - Fork 548
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
D1: execute --file --remote now uses dedicated import API #5696
Conversation
🦋 Changeset detectedLatest commit: cf78a40 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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.devprod.cloudflare.dev/workers-sdk/runs/9112753485/npm-package-wrangler-5696 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5696/npm-package-wrangler-5696 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9112753485/npm-package-wrangler-5696 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9112753485/npm-package-create-cloudflare-5696 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9112753485/npm-package-cloudflare-kv-asset-handler-5696 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9112753485/npm-package-miniflare-5696 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9112753485/npm-package-cloudflare-pages-shared-5696 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9112753485/npm-package-cloudflare-vitest-pool-workers-5696 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
60c250b
to
b978e8d
Compare
packages/wrangler/package.json
Outdated
@@ -172,6 +172,7 @@ | |||
"jest": "^29.7.0", | |||
"jest-fetch-mock": "^3.0.3", | |||
"jest-websocket-mock": "^2.5.0", | |||
"md5-file": "^5.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a dependency for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're expecting 5-10GB files so I had a look around for something that had a good implementation for larger files. Would be pretty easy to vendor the particular code path we use, if you'd prefer. For now I'll just pin to the exact version at least.
async function d1ApiPost<T>( | ||
accountId: string, | ||
db: Database, | ||
action: string, | ||
body: unknown | ||
) { | ||
return await fetchResult<T>( | ||
`/accounts/${accountId}/d1/database/${db.uuid}/${action}`, | ||
{ | ||
method: "POST", | ||
headers: { | ||
"Content-Type": "application/json", | ||
...(db.internal_env ? { "x-d1-internal-env": db.internal_env } : {}), | ||
}, | ||
body: JSON.stringify(body), | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? We already have utility methods for calling the Cloudflare API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that what fetchResult
is? I just extracted this so that all staging DB calls sent the right header, but if there's another internal api I should use for doing the fetch then lmk
); | ||
} | ||
|
||
async function pollUntilComplete( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if this could display a spinner UI like Cloudchamber
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a util called spinnerWhile
which you can wrap you async stuff in pretty easily to get the spinner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
113323a
to
1130964
Compare
This was only required for `execute --remote --file`, which will now have a dedicated endpoint.
This hits a new API endpoint 'import', that initially returns a signed R2 PUT url for uploading SQL, then returns polling info as the import completes. This completely bypasses the existing split-chunk-and-execute implementation that often left your DB in an invalid state
What this PR solves / how to test
Fixes CFSQL-794:
This hits a new API endpoint 'import', that initially returns a signed R2 PUT url for uploading SQL, then returns polling info as the import completes. This completely bypasses the existing split-chunk-and-execute implementation that often left your DB in an invalid state
D1 server-side code is up and running so this should be fully functional for testing & feedback.
Author has addressed the following