[tm_state] updateLocked should re-populate local metadata tables to reflect promotion rule changes by ajm188 · Pull Request #8107 · vitessio/vitess

ajm188 · 2021-05-12T14:09:46Z

Signed-off-by: Andrew Mason amason@slack-corp.com

Description

TODO.

This means we call PopulateMetadataTables on:

(*tmState).ChangeTabletType
(*tmState).RefreshFromTopoInfo
(*tmState).RefreshFromTopo (via RefreshFromTopoInfo)
(*tmState).Open

See it in action

❯ vtctldclient --server "localhost:15999" GetTablets
zone1-0000000100 commerce 0 master SFO-M-AMASON02:15100 SFO-M-AMASON02:17100 [] 2021-05-12T15:48:34Z
zone1-0000000101 commerce 0 replica SFO-M-AMASON02:15101 SFO-M-AMASON02:17101 [] <null>
zone1-0000000102 commerce 0 rdonly SFO-M-AMASON02:15102 SFO-M-AMASON02:17102 [] <null>

# on zone1-101
mysql> select * from _vt.local_metadata;
+---------------+------------------+-------------+
| name          | value            | db_name     |
+---------------+------------------+-------------+
| Alias         | zone1-0000000101 | vt_commerce |
| ClusterAlias  | commerce.0       | vt_commerce |
| DataCenter    | zone1            | vt_commerce |
| PromotionRule | neutral          | vt_commerce |
+---------------+------------------+-------------+
4 rows in set (0.00 sec)

# now, the tablet type change
❯ vtctldclient --server "localhost:15999" ChangeTabletType zone1-0000000101 rdonly
- zone1-0000000101 commerce 0 replica SFO-M-AMASON02:15101 SFO-M-AMASON02:17101 [] <null>
+ zone1-0000000101 commerce 0 rdonly SFO-M-AMASON02:15101 SFO-M-AMASON02:17101 [] <null>

mysql> mysql> select * from _vt.local_metadata;
+---------------+------------------+-------------+
| name          | value            | db_name     |
+---------------+------------------+-------------+
| Alias         | zone1-0000000101 | vt_commerce |
| ClusterAlias  | commerce.0       | vt_commerce |
| DataCenter    | zone1            | vt_commerce |
| PromotionRule | must_not         | vt_commerce |
+---------------+------------------+-------------+
4 rows in set (0.01 sec)

Related Issue(s)

Checklist

Tests were added or are not required
Documentation was added or is not required

Deployment Notes

rafael · 2021-05-13T00:22:04Z

go/vt/vttablet/tabletmanager/tm_state.go

Populate metadata does some extra work that only makes sense when the tablet is bootstrapping (like creating the metadata table). All the instructions are idempotent, but I think it will be clearer/cleaner to explicitly add a method to only update the metadata

ajm188 · 2021-05-13T02:15:38Z

Ugh, I've fixed (mostly, there are some flaky tests) all the unit tests, but this is very much breaking the faketablet/fakesqldb invariants expected by the legacy workflow tests, and I'm not sure how to best tackle that

ajm188 · 2021-05-14T00:32:03Z

At last!! Only the true failures:

    tablet_test.go:107: 
        	Error Trace:	tablet_test.go:107
        	Error:      	Expected nil, but got: [][]sqltypes.Value{[]sqltypes.Value{sqltypes.Value{typ:6165, val:[]uint8{0x41, 0x6c, 0x69, 0x61, 0x73}}, sqltypes.Value{typ:10260, val:[]uint8{0x7a, 0x6f, 0x6e, 0x65, 0x31, 0x2d, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x38, 0x36}}, sqltypes.Value{typ:10262, val:[]uint8{0x76, 0x74, 0x5f, 0x6b, 0x73}}}, []sqltypes.Value{sqltypes.Value{typ:6165, val:[]uint8{0x43, 0x6c, 0x75, 0x73, 0x74, 0x65, 0x72, 0x41, 0x6c, 0x69, 0x61, 0x73}}, sqltypes.Value{typ:10260, val:[]uint8{0x6b, 0x73, 0x2e, 0x30}}, sqltypes.Value{typ:10262, val:[]uint8{0x76, 0x74, 0x5f, 0x6b, 0x73}}}, []sqltypes.Value{sqltypes.Value{typ:6165, val:[]uint8{0x44, 0x61, 0x74, 0x61, 0x43, 0x65, 0x6e, 0x74, 0x65, 0x72}}, sqltypes.Value{typ:10260, val:[]uint8{0x7a, 0x6f, 0x6e, 0x65, 0x31}}, sqltypes.Value{typ:10262, val:[]uint8{0x76, 0x74, 0x5f, 0x6b, 0x73}}}, []sqltypes.Value{sqltypes.Value{typ:6165, val:[]uint8{0x50, 0x72, 0x6f, 0x6d, 0x6f, 0x74, 0x69, 0x6f, 0x6e, 0x52, 0x75, 0x6c, 0x65}}, sqltypes.Value{typ:10260, val:[]uint8{0x6e, 0x65, 0x75, 0x74, 0x72, 0x61, 0x6c}}, sqltypes.Value{typ:10262, val:[]uint8{0x76, 0x74, 0x5f, 0x6b, 0x73}}}}
        	Test:       	TestLocalMetadata

Looks like I will need to some additional plumbing of state through to tell whether we're initializing or going through a state transition, which will also help incorporating your suggestion @rafael simpler.

…eflect promotion rule changes Signed-off-by: Andrew Mason <amason@slack-corp.com>

If we're enabling the queryservice, wait until afterwards to populate the metadata. On the other hand, if we're disabling the queryservice, then populate the metadata just before shutting down the queryservice. Signed-off-by: Andrew Mason <amason@slack-corp.com>

Signed-off-by: Andrew Mason <amason@slack-corp.com>

…ate_metadata` flag Signed-off-by: Andrew Mason <amason@slack-corp.com>

Signed-off-by: Andrew Mason <amason@slack-corp.com>

…ator func Also, track if we've previously called PopulateMetadataTables, and if so, only do the Upsert rather than Create+Upsert Signed-off-by: Andrew Mason <amason@slack-corp.com>

shlomi-noach

@ajm188 nice work and thank you for the contribution! Code looks good, I do wish to reduce some confusion about function names and visibility of functions, please see inline comments.

shlomi-noach · 2021-05-18T09:40:31Z

go/vt/mysqlctl/metadata_tables.go

 // Vitess.
-func PopulateMetadataTables(mysqld MysqlDaemon, localMetadata map[string]string, dbName string) error {
-	log.Infof("Populating _vt.local_metadata table...")
+func CreateMetadataTables(mysqld MysqlDaemon, dbName string) error {


It seems like this function is only called locally, rename it to createMetadataTables()?

shlomi-noach · 2021-05-18T09:42:15Z

go/vt/mysqlctl/metadata_tables.go

+	return createMetadataTables(conn, dbName)
+}
+
+func createMetadataTables(conn *dbconnpool.DBConnection, dbName string) error {


... following up on my previous comment, we now have one method CreateMetadataTables, one function CreateMetadataTables, one function createMetadataTables. Let's try and make these less ambiguous by renaming the latter two?

Yeah, how about this:

MetadataManager has public methods: {Create,Populate}MetadataTables and UpsertMetadataTables

The main reason I did this "public function and also public method on struct" thing was to reduce the number of changes I needed to do (because otherwise I would have to update everyone that called mysqlctl.PopulateMetadataTables .... which now that i look is just two places in mysqlctl/backup.go so maybe I should just change them 🤔 )

The other reason for the public/private with the same name was to allow code reuse without having to get new dba connections for each stage, so in Populate, we first get a connection, and then call private create and private upsert, whereas if we called public Create and public Upsert then we would get a new connection for each of those.

Then, we do:

type MetadataManager struct {} func (m *MetadataManager) PopulateMetadataTables(mysqld MysqldDaemon, dbName string) error { log.Infof("....") conn, err := mysqld.GetDbaConnection(...) if err != nil { return err } defer conn.Close() return m.populateMetadataTablesWithConn(conn, dbName) } func PopulateMetadataTables(mysqld MysqldDaemon, localMetadata map[string]string, dbName string) error { m := &MetadataManager{} return m.PopulateMetadataTables(mysqld, localMetadata, dbName) }

and then the private function does all the logic, and has a slightly cleaner naming (due to the "WithConn" suffix, or at least that's my goal!). And if we want, I can update those last few callsites and remove the package-level public function as well.

What do you think?

Sounds good. We can do so iteratively in followup PRs.

shlomi-noach · 2021-05-18T09:44:13Z

go/vt/mysqlctl/metadata_tables.go

+// Callers are responsible for ensuring the _vt.local_metadata table exists
+// before calling this function, usually by calling CreateMetadataTables at
+// least once prior.
+func UpsertLocalMetadata(mysqld MysqlDaemon, localMetadata map[string]string, dbName string) error {


this function is only called locally, reduce its visibility by lowercasing it?

shlomi-noach · 2021-05-18T09:44:48Z

go/vt/mysqlctl/metadata_tables.go

+	return upsertLocalMetadata(conn, localMetadata, dbName)
+}
+
+func upsertLocalMetadata(conn *dbconnpool.DBConnection, localMetadata map[string]string, dbName string) error {


similarly, we have three different functions now called upsertLocalMetadata with different capitalization. Let's change names to avoid confusion.

ajm188 requested a review from rafael May 12, 2021 14:09

ajm188 force-pushed the am_populate_metadata_on_type_change branch from 7881b90 to 9953560 Compare May 12, 2021 14:13

ajm188 changed the title ~~[tm_state] updateLocked should re-populated local metadata tables to reflect promotion rule changes~~ [tm_state] updateLocked should re-populate local metadata tables to reflect promotion rule changes May 12, 2021

ajm188 force-pushed the am_populate_metadata_on_type_change branch 2 times, most recently from 85e47b9 to 217fd18 Compare May 13, 2021 00:05

rafael reviewed May 13, 2021

View reviewed changes

ajm188 force-pushed the am_populate_metadata_on_type_change branch from 1824722 to 765346d Compare May 15, 2021 02:30

Andrew Mason added 7 commits May 14, 2021 22:34

[tm_state] updateLocked should re-populate local metadata tables to r…

3890523

…eflect promotion rule changes Signed-off-by: Andrew Mason <amason@slack-corp.com>

Update testTM mysqldaemon to support local_metadata queries

a407234

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Add missing updateLocked call on tablettype change in test

049d4b5

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Track whether tm_state is opening, and try to respect the `init_popul…

449f852

…ate_metadata` flag Signed-off-by: Andrew Mason <amason@slack-corp.com>

Separate metadata table creation from upserting

6d9af98

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Add a MetadataManager to replace the TabletManager.LocalMetadataPopul…

cf15b31

…ator func Also, track if we've previously called PopulateMetadataTables, and if so, only do the Upsert rather than Create+Upsert Signed-off-by: Andrew Mason <amason@slack-corp.com>

ajm188 force-pushed the am_populate_metadata_on_type_change branch from 765346d to cf15b31 Compare May 15, 2021 02:35

ajm188 mentioned this pull request May 17, 2021

[tm_state] updateLocked should re-populate local metadata tables to reflect promotion rule changes tinyspeck/vitess#209

Merged

7 tasks

ajm188 marked this pull request as ready for review May 17, 2021 23:27

ajm188 requested review from deepthi and shlomi-noach as code owners May 17, 2021 23:27

shlomi-noach reviewed May 18, 2021

View reviewed changes

shlomi-noach approved these changes May 18, 2021

View reviewed changes

rafael merged commit e792b20 into vitessio:master May 18, 2021

ajm188 mentioned this pull request May 20, 2021

[mysqlctl] Restructure MetadataManager to reduce public API surface area #8152

Merged

2 tasks

ajm188 deleted the am_populate_metadata_on_type_change branch May 29, 2021 10:24

askdba added Component: Cluster management Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Jun 10, 2021

deepthi mentioned this pull request Jun 22, 2021

tm state: don't populate metadata in updateLocked #8362

Merged

2 tasks

aquarapid mentioned this pull request Jul 20, 2021

Non-optional sql_log_bin=0 in PopulateMetadataTables leads to error on external tablets without SUPER privileges #8505

Closed

deepthi mentioned this pull request Oct 7, 2021

Add restore_extra_restart_args for restore #8929

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tm_state] updateLocked should re-populate local metadata tables to reflect promotion rule changes#8107

[tm_state] updateLocked should re-populate local metadata tables to reflect promotion rule changes#8107
rafael merged 7 commits intovitessio:masterfrom
tinyspeck:am_populate_metadata_on_type_change

ajm188 commented May 12, 2021 •

edited

Loading

Uh oh!

rafael May 13, 2021

Uh oh!

ajm188 commented May 13, 2021

Uh oh!

ajm188 commented May 14, 2021

Uh oh!

shlomi-noach left a comment

Uh oh!

shlomi-noach May 18, 2021

Uh oh!

shlomi-noach May 18, 2021

Uh oh!

ajm188 May 18, 2021 •

edited

Loading

Uh oh!

shlomi-noach May 18, 2021

Uh oh!

shlomi-noach May 18, 2021

Uh oh!

shlomi-noach May 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ajm188 commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

See it in action

Related Issue(s)

Checklist

Deployment Notes

Uh oh!

rafael May 13, 2021

Choose a reason for hiding this comment

Uh oh!

ajm188 commented May 13, 2021

Uh oh!

ajm188 commented May 14, 2021

Uh oh!

shlomi-noach left a comment

Choose a reason for hiding this comment

Uh oh!

shlomi-noach May 18, 2021

Choose a reason for hiding this comment

Uh oh!

shlomi-noach May 18, 2021

Choose a reason for hiding this comment

Uh oh!

ajm188 May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shlomi-noach May 18, 2021

Choose a reason for hiding this comment

Uh oh!

shlomi-noach May 18, 2021

Choose a reason for hiding this comment

Uh oh!

shlomi-noach May 18, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ajm188 commented May 12, 2021 •

edited

Loading

ajm188 May 18, 2021 •

edited

Loading