VS-197: Enable wrangler to interact with Vectorize V2 indexes#6252
VS-197: Enable wrangler to interact with Vectorize V2 indexes#6252RamIdeas merged 1 commit intocloudflare:mainfrom
Conversation
🦋 Changeset detectedLatest commit: a7a4f39 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 |
56dbe68 to
66bfefa
Compare
|
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/10209363038/npm-package-wrangler-6252You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6252/npm-package-wrangler-6252Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10209363038/npm-package-wrangler-6252 dev path/to/script.jsAdditional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10209363038/npm-package-create-cloudflare-6252 --no-auto-updatenpm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10209363038/npm-package-cloudflare-kv-asset-handler-6252npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10209363038/npm-package-miniflare-6252npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10209363038/npm-package-cloudflare-pages-shared-6252npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10209363038/npm-package-cloudflare-vitest-pool-workers-6252Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
petebacondarwin
left a comment
There was a problem hiding this comment.
I was a bit confused by the use of v2 at first.
I think what you are saying is that the v2 flag effectively provides a brand new product from v1, right?
For example it seems that you can create v1 and v2 indexes with the same name and then interact with them separately?
If not then what would it mean to create a v1 index and then use v2 commands to interact with it?
Can you clarify this in the command docs?
.changeset/thick-hairs-notice.md
Outdated
There was a problem hiding this comment.
| "wrangler": patch | |
| "wrangler": minor |
There was a problem hiding this comment.
Vectorize V1 is an open-beta product, and Vectorize V2 is a re-architectured system that will soon go beta and eventually GA (at which point we will deprecate Vectorize v1).
Vectorize V2 is essentially a different product. V2 operations can only work on a v2 index (created using the use-v2 flag) and the same applies for V1. However, we cannot create a v2 index if a v1 index of the same name exists and vice versa.
There was a problem hiding this comment.
Updated the changeset patch to minor, and made the v2 option description in the commands more descriptive. @petebacondarwin I hope that would be sufficient.
There was a problem hiding this comment.
What does v2 mean when it comes to indexes?
What would happen if test-index was not v2?
There was a problem hiding this comment.
If we try a v2 operation (say get index) on a v1 index, the wrangler client would receive a 403 response from Vectorize. Same goes if we try a v1 operation on a v2 index.
There was a problem hiding this comment.
What would be the error message. Will it be clear to the user what went wrong and what to do to fix it?
Also what is the roll off plan for deprecating and removing v1?
There was a problem hiding this comment.
For example, for how long will you support use-v2 as a command line flag?
In general we are moving towards feature flagging things in Wrangler with the experimental/x prefix. So in this case it would be
wrangler vectorize get test-index --experimental-v2
or
wrangler vectorize get test-index --x-v2
And for stuff you are removing prefixing with legacy/deprecated. For example:
wrangler vectorize get test-index --legacy-v1
or
wrangler vectorize get test-index --deprecated-v1
There was a problem hiding this comment.
The error message in the apiv4 response format would be like Index: "${index_name}" with version "${index_version}" called with api version "${operation_version}" along with a 403 response.
I would let Yevgen address the deprecation plan.
There was a problem hiding this comment.
For example, for how long will you support
use-v2as a command line flag? In general we are moving towards feature flagging things in Wrangler with theexperimental/xprefix. So in this case it would bewrangler vectorize get test-index --experimental-v2or
wrangler vectorize get test-index --x-v2And for stuff you are removing prefixing with
legacy/deprecated. For example:wrangler vectorize get test-index --legacy-v1or
wrangler vectorize get test-index --deprecated-v1
@petebacondarwin the plan to start with private beta this month, once we will be confident to open it up for public beta sometime in August we will stop allowing V1 index creation and V2 only will be used for new indexes.
There was a problem hiding this comment.
I like the idea of using --deprecated-v1 for dealing with existing V1 indexes once V2 goes to public beta.
There was a problem hiding this comment.
We discussed this within our team, and decided to with an --experimental-v2 flag with a default value of false until we put v1 indexes on the deprecation path. At that time we will switch to --deprecated-v1.
Made the change to update the commands and the related docs.
There was a problem hiding this comment.
We discussed this within our team, and decided to with an
--experimental-v2flag with a default value of false until we put v1 indexes on the deprecation path. At that time we will switch to--deprecated-v1.Made the change to update the commands and the related docs.
This is not what I see in the current code for this PR.
I see that there is no --experimental-v2 flag here only the --deprecated-v1 flag, which defaults to false.
I would assume that for the private beta the default needs to switch to true and the --experimental-v2 flag needs to be implemented?
Also, could I call out that it would be much nicer for the user if these commands inferred the version of the index from its type (via a REST request) rather than requiring the user to keep track of which version they are using. The only place one really needs the v1/v2 flag is at index creation time, no?
66bfefa to
6921707
Compare
|
Is there a command that converts a v1 index to a v2 index? |
This will be worked on later in the quarter, definitely something we will provide before GA. |
6921707 to
8997c93
Compare
|
@garvit-gupta is my understanding correct that some backend work needs to happen before we can merge & release this PR? Are any further wrangler changes required or is the PR in its current state ready to go? Can you please let us know when we should revisit this PR to merge it 🙏 ? |
8997c93 to
7fbe3e7
Compare
|
spoke async with the team, and this is now ready for re-review |
88ce511 to
719cfad
Compare
|
As I understand it, this PR should only land when we V2 goes into public beta. |
719cfad to
587abd8
Compare
ndisidore
left a comment
There was a problem hiding this comment.
This is looking great! I know there's some work you want to do around the query parse function. Happy to approve once we look at that
587abd8 to
04eb095
Compare
The query filter parse and validate logic looks cleaner now. Ready for review! |
|
@petebacondarwin @CarmenPopoviciu could you please confirm this PR is ready to be merged from your perspective? Confirmed, we want to release this change once Vectorize V2 goes into public beta. |
petebacondarwin
left a comment
There was a problem hiding this comment.
What a massive PR! From what I can see this looks great.
Happy for this to be merged when it is unblocked for product release.
cc @RamIdeas - you might be interested in this contribution.
04eb095 to
7a6a5d9
Compare
7a6a5d9 to
a7a4f39
Compare
|
@RamIdeas @petebacondarwin we are happy to merge the change and release it next week. |
|
Congratulations @garvit-gupta, the maintainer of this repository has issued you a holobyte! Here it is: https://holopin.io/holobyte/clzr77nwc143120cleznufaycs This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account. |
What this PR solves / how to test
Enables users to operate on Vectorize V2 operations via wrangler.
Fixes #[VS-197].
Author has addressed the following