vindex & vrepl: multi-column support#5509
Merged
sougou merged 5 commits intovitessio:masterfrom Dec 10, 2019
Merged
Conversation
This new change extends the Vindex protocol slightly differently from the previous approach. In the previous approach, a Vindex had the option of supporting an additional MultiColumn Map API on top of the existing single column Map. This allowed for a Vindex to be a functional one through the MultiColumn API, and a lookup one through the regular Map. But the same functionality can be achieved by two vindexes, and leads to a more flexible and composable design. In the new design, a Vindex can decide if it wants to provide a SingleColumn of MultiColumn API. If the caller is capable of using the MultiColumn API, they use it. Otherwise, they treat the vindex as non-existent. This is the initial cut. With this change, we can bring back the restriction to disallow owned vindexes from being primary. Insert and VReplication will use MultiColumn. The rest of v3 will continue to use SingleColumn for now. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
sougou
force-pushed
the
ss-vrepl-multicol
branch
from
9502a71 to
1c50ee9
Compare
deepthi
approved these changes
Dec 6, 2019
Collaborator
deepthi
left a comment
deepthi
left a comment
There was a problem hiding this comment.
Nice clean design. Better than the previous iteration.
A couple of nits and a question inline.
go/vt/vtgate/vindexes/vindex.go
Outdated
| @@ -142,18 +148,24 @@ func CreateVindex(vindexType, name string, params map[string]string) (Vindex, er | |||
|
|
|||
| // Map invokes MapMulti or Map depending on which is available. | |||
go/vt/vtgate/vindexes/vindex.go
Outdated
| return nil, vterrors.New(vtrpcpb.Code_INTERNAL, "vindex does not have Map functions") | ||
| } | ||
|
|
||
| // Verify invokes VerifyMulti or Verify depending on which is available. |
| return i, &keyspaceIDResolverFactoryV3{ | ||
| vindex: colVindex.Vindex, | ||
| // Only SingleColumn vindexes are returned by FindVindexForSharding. | ||
| vindex: colVindex.Vindex.(vindexes.SingleColumn), |
Collaborator
There was a problem hiding this comment.
Can bad things happen if this turns out to be nil?
Contributor
Author
There was a problem hiding this comment.
The previous function guarantees that the value won't be nil.
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
This was referenced Dec 17, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This new change extends the Vindex protocol slightly differently
from the previous approach.
In the previous approach, a Vindex had the option of supporting
an additional MultiColumn Map API on top of the existing single
column Map. This allowed for a Vindex to be a functional one
through the MultiColumn API, and a lookup one through the regular
Map.
But the same functionality can be achieved by two vindexes, and
leads to a more flexible and composable design.
In the new design, a Vindex can decide if it wants to provide
a SingleColumn of MultiColumn API. If the caller is capable of
using the MultiColumn API, they use it. Otherwise, they treat
the vindex as non-existent.
This is the initial cut. With this change, we can bring back
the restriction to disallow owned vindexes from being primary.
Insert and VReplication use MultiColumn. The rest of v3
will continue to use SingleColumn for now.