-
Notifications
You must be signed in to change notification settings - Fork 2.3k
LookupVindex: Multiple lookup tables support for LookupVindexCreate
#17566
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
LookupVindex: Multiple lookup tables support for LookupVindexCreate
#17566
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Noble Mittal <[email protected]>
d51393b to
c9e74bf
Compare
Signed-off-by: Noble Mittal <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17566 +/- ##
==========================================
- Coverage 67.51% 67.49% -0.03%
==========================================
Files 1601 1601
Lines 261576 262037 +461
==========================================
+ Hits 176614 176867 +253
- Misses 84962 85170 +208 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
mattlord
left a comment
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.
IMO we should not allow people to create workflows that they cannot finish (cancel, externalize, complete). I think that it should be done all at once.
go/cmd/vtctldclient/command/vreplication/lookupvindex/lookupvindex.go
Outdated
Show resolved
Hide resolved
go/cmd/vtctldclient/command/vreplication/lookupvindex/lookupvindex.go
Outdated
Show resolved
Hide resolved
go/cmd/vtctldclient/command/vreplication/lookupvindex/lookupvindex.go
Outdated
Show resolved
Hide resolved
go/cmd/vtctldclient/command/vreplication/lookupvindex/lookupvindex.go
Outdated
Show resolved
Hide resolved
go/cmd/vtctldclient/command/vreplication/lookupvindex/lookupvindex.go
Outdated
Show resolved
Hide resolved
efe9c4e to
ae3b4d1
Compare
Signed-off-by: Noble Mittal <[email protected]>
ae3b4d1 to
4921626
Compare
…multiple lv Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
…plete Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
rohit-nayak-ps
left a comment
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.
Nice work. Let's immediately update the website docs for this.
| Type: "lookup_unique", | ||
| Params: map[string]string{ | ||
| "table": "targetks.lookup", | ||
| "from": "c1", |
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.
Shouldn't "from" refer to a column in the source table?
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.
yeah, but I don't think it matters here, since we anyway expect the same in wantvschema. but, updated it since it should be reasonable in unit tests. also, pushed better query matching with expectVRQuery (please have a look at f522479)
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
|
Here's the docs PR: vitessio/website#1964 |
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Description
params-fileis added increatecommand. The sampleJSONfile is given below. We will throw error if the command includes bothparams-fileand other vindex-param flags.externalize,internalizeandcompletecommand ofLookupVindex.LookupVindexeshas been added to theWorkflowOptions. To support old behaviour, we find emptyLookupVindexesslice from options, we assume that the name of lookup vindex is same as workflow name.LookupVindexCreate,LookupVindexExternalize,LookupVindexInternalize,LookupVindexCompleteare added.Testing
LookupVindexcommand:vindex_params.jsonfile:{ "corder_lookup": { "lookup_vindex_type": "consistent_lookup_unique", "table_owner": "corder", "table_owner_columns": [ "sku" ] }, "customer_lookup": { "lookup_vindex_type": "consistent_lookup_unique", "table_owner": "customer", "table_owner_columns": [ "email" ] } }Related Issue(s)
Checklist
Deployment Notes