Allow setting key/value in a metadata stored in the topology #5103

kalfonso · 2019-08-16T05:06:44Z

This PR allows setting key/value metadata in the topology leveraging "set" & "show" SQL statements.

Signed-off-by: Karel Alfonso Sague kalfonso@squareup.com

kalfonso · 2019-08-16T05:11:46Z

@sougou @demmer @tirsen this is an initial cut of using "SET" statement to manage metadata values. Looking for feedback on this. I've commented in the PR about alternative options to manage topology backed metadata

go/vt/vtgate/executor.go

alecthomas · 2019-08-21T06:31:52Z

go/vt/topo/metadata.go

+
+ keyPattern := strings.Replace(regexp.QuoteMeta(keyFilter), "%", ".*", -1)
+ keyPattern = fmt.Sprintf("^%s$", keyPattern)
+ re := regexp.MustCompile(keyPattern)


I'd add a little comment at the end like // Can never fail.

alecthomas · 2019-08-21T06:32:41Z

go/vt/vtgate/executor.go

@@ -465,6 +465,11 @@ func (e *Executor) handleSet(ctx context.Context, safeSession *SafeSession, sql
 if k.Scope == sqlparser.GlobalStr {
 return &sqltypes.Result{}, vterrors.New(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported in set: global")
 }
+
+ if k.Scope == sqlparser.VitessMetadataStr {


Use a switch here now that you have >1 case?

alecthomas · 2019-08-21T06:34:32Z

go/vt/vtgate/executor_test.go

+
+ show := "show vitess_metadata variables like 'app_keyspace%'"
+ result, err := executor.Execute(context.Background(), "TestExecute", session, show, nil)
+ if err != nil {


Is testify not used in Vitess? :(

It seems to be used only in one test go/vt/vtgate/engine/ordered_aggregate_test.go

Yeah, I just introduced it, and used it on the code I was working on at the time. You can and should definitely use it. :)

Done. Used in the new tests

alecthomas · 2019-08-21T06:35:51Z

go/vt/vtgate/executor.go

+ }
+ }
+
+ rows := make([][]sqltypes.Value, 0, 2)


I'd use len(metadata) as the capacity here.

tirsen · 2019-08-21T22:47:17Z

go/vt/vtgate/executor_test.go

+ t.Errorf("want migrations %v, result %v", wantqr, gotqr)
+ }
+
+ show = "show vitess_metadata variables"


oh I didn't know the variables keyword. I assume this is "standard"?

Yes, wanted to keep same syntax

SHOW [GLOBAL | SESSION] VARIABLES [LIKE 'pattern' | WHERE expr]

https://dev.mysql.com/doc/refman/8.0/en/show-variables.html

tirsen · 2019-08-21T22:48:15Z

go/vt/vtgate/executor.go

@@ -680,6 +735,10 @@ func (e *Executor) handleShow(ctx context.Context, safeSession *SafeSession, sql

 switch show.Type {
 case sqlparser.KeywordString(sqlparser.COLLATION), sqlparser.KeywordString(sqlparser.VARIABLES):
+ if show.Scope == sqlparser.VitessMetadataStr {
+ return e.handleShowVitessMetadata(ctx, safeSession, destKeyspace, show.ShowTablesOpt)


You're still passing in the keyspace here?

tirsen · 2019-08-21T22:48:35Z

go/vt/vtgate/executor.go

+ return &sqltypes.Result{}, nil
+}
+
+func (e *Executor) handleShowVitessMetadata(ctx context.Context, session *SafeSession, ksName string, opt *sqlparser.ShowTablesOpt) (*sqltypes.Result, error) {


I think you're not using ksName?

kalfonso · 2019-08-23T05:23:49Z

go/vt/vtgate/executor.go

@@ -645,6 +649,63 @@ func (e *Executor) handleSet(ctx context.Context, safeSession *SafeSession, sql
 return &sqltypes.Result{}, nil
 }

+func (e *Executor) handleSetVitessMetadata(ctx context.Context, session *SafeSession, k sqlparser.SetKey, v interface{}) (*sqltypes.Result, error) {
+ //TODO(kalfonso): move to its own acl check and consolidate into an acl component that can handle multiple operations (vschema, metadata)
+ allowed := vschemaacl.Authorized(callerid.ImmediateCallerIDFromContext(ctx))


@dweitzman I've made changes to use vschemaacl to authorize specific users to execute "set vitess_metadata" actions. This works for our current requirements but a separate PR will separate this into its own metadataacl definition. Trying to keeps this PR small

demmer

Overall this LGTM -- one minor comment but I'm happy with this syntax and approach.

demmer · 2019-08-25T19:20:01Z

go/vt/topo/metadata.go

+
+ keyPattern := strings.Replace(regexp.QuoteMeta(keyFilter), "%", ".*", -1)
+ keyPattern = fmt.Sprintf("^%s$", keyPattern)
+ re := regexp.MustCompile(keyPattern) // Can never fail


Are we 100% sure this is a compatible conversion from the LIKE syntax to regexp? Itt's fine for this use case but if there's a more robust and generic way to do this, we might get some mileage out of making it a common function in the Vitess code base.

I found this link with examples of the LIKE syntax: https://www.w3schools.com/sql/sql_like.asp

I we also need to support _.

I agree that extracting a "LIKE to regexp" converter into a separate file and a more shared place and then writing specific unit tests for that would be nice. I'm not sure we need to make that 100% compliant in this PR though. We can iterate on that once we have it in a central place with a nice suite of unit tests.

Yep -- totally agree 100%

Extracted functionality to separate file with unit tests. Also supporting '_' character and escaping using ""

Signed-off-by: Karel Alfonso Sague <kalfonso@squareup.com>

sougou

We also need the ability to delete metadata. How about we treat an empty value as a delete?

sougou · 2019-08-28T16:37:33Z

go/vt/topo/server.go

@@ -84,6 +84,7 @@ const (
 KeyspacesPath = "keyspaces"
 ShardsPath = "shards"
 TabletsPath = "tablets"
+ MetadataPath = "vitess_metadata"


Hope it's not too late. This should be just metadata, just like we didn't prefix the other paths with vitess_.

It's not too late. This is the path in zk right? Yes I agree this should be just metadata not vitess_metadata. The syntax should be SET @@vitess_metadata though just like all of our other SQL extensions. Right?

Yes. We don't want collisions in the variable name.

Renamed in this PR: #5141

kalfonso force-pushed the kalfonso.190814-set-vitess-metadata branch 6 times, most recently from cdfdfac to aa73834 Compare August 20, 2019 05:58

tirsen requested changes Aug 20, 2019

View reviewed changes

go/vt/vtgate/executor.go Outdated Show resolved Hide resolved

kalfonso force-pushed the kalfonso.190814-set-vitess-metadata branch from aa73834 to d4f59c0 Compare August 21, 2019 04:12

kalfonso changed the title ~~WIP: Allow setting key/value in a metadata stored in the topology~~ Allow setting key/value in a metadata stored in the topology Aug 21, 2019

kalfonso force-pushed the kalfonso.190814-set-vitess-metadata branch from d4f59c0 to 99eb8b7 Compare August 21, 2019 05:54

kalfonso marked this pull request as ready for review August 21, 2019 05:54

kalfonso requested a review from sougou as a code owner August 21, 2019 05:54

alecthomas reviewed Aug 21, 2019

View reviewed changes

tirsen approved these changes Aug 21, 2019

View reviewed changes

kalfonso force-pushed the kalfonso.190814-set-vitess-metadata branch 3 times, most recently from a8b2f6c to de15fda Compare August 23, 2019 03:37

kalfonso commented Aug 23, 2019

View reviewed changes

kalfonso force-pushed the kalfonso.190814-set-vitess-metadata branch from de15fda to a25abf2 Compare August 23, 2019 06:03

demmer reviewed Aug 25, 2019

View reviewed changes

kalfonso force-pushed the kalfonso.190814-set-vitess-metadata branch 3 times, most recently from 40b7aef to 3bc0486 Compare August 26, 2019 13:55

Allow setting key/value metadata stored in the topology

453d54b

Signed-off-by: Karel Alfonso Sague <kalfonso@squareup.com>

kalfonso force-pushed the kalfonso.190814-set-vitess-metadata branch from 3bc0486 to 453d54b Compare August 26, 2019 14:26

tirsen merged commit ad6099c into vitessio:master Aug 28, 2019

sougou reviewed Aug 28, 2019

View reviewed changes

spark4 mentioned this pull request Nov 13, 2019

Serry deploy tinyspeck/vitess#140

Closed

spark4 mentioned this pull request Nov 22, 2019

Slack sync upstream 2019 11 09.r0 tinyspeck/vitess#142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting key/value in a metadata stored in the topology #5103

Allow setting key/value in a metadata stored in the topology #5103

kalfonso commented Aug 16, 2019 •

edited

Loading

kalfonso commented Aug 16, 2019

alecthomas Aug 21, 2019

kalfonso Aug 22, 2019

alecthomas Aug 21, 2019

kalfonso Aug 22, 2019

alecthomas Aug 21, 2019

kalfonso Aug 23, 2019

systay Aug 23, 2019

kalfonso Aug 26, 2019

alecthomas Aug 21, 2019

kalfonso Aug 22, 2019

tirsen Aug 21, 2019

kalfonso Aug 22, 2019

tirsen Aug 21, 2019

kalfonso Aug 22, 2019

tirsen Aug 21, 2019

kalfonso Aug 22, 2019

kalfonso Aug 23, 2019

demmer left a comment

demmer Aug 25, 2019

tirsen Aug 26, 2019

demmer Aug 26, 2019

kalfonso Aug 26, 2019

sougou left a comment

sougou Aug 28, 2019

tirsen Aug 29, 2019

sougou Aug 29, 2019

kalfonso Aug 30, 2019

Allow setting key/value in a metadata stored in the topology #5103

Allow setting key/value in a metadata stored in the topology #5103

Conversation

kalfonso commented Aug 16, 2019 • edited Loading

kalfonso commented Aug 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sougou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalfonso commented Aug 16, 2019 •

edited

Loading