Simplifying topo implementations. #3402

alainjobart · 2017-11-17T18:29:58Z

Now that all implementations are based on the topo.Backend sub-interface, this is possible.

BUG=30274258

This is moving up the code that was exactly the same from all topos up to the topo.Server object. This is now possible, since all topo implementations had the same code. The Tee code needs to be fixed so the backend code works correctly with the Backend interface, will do that in the next commit. BUG=30274258

Just want to make sure the version code works, as it is different for the topo.Backend methods than it was for the *Keyspace methods.

And fixing related issues.

In the process, converting the discovery topology watcher code to use a regular memorytopo, instead of a custom topo.

No need to have the interface now that we only have one implementation. Moved everything into vtctld/explorer.go.

alainjobart · 2017-11-19T21:13:11Z

@sougou ready for review. Probably one commit at a time is easier.

(will create an internal CL for it too)

alainjobart · 2017-11-19T21:13:39Z

And this: +938 −3,855 yes!

pivanof · 2017-11-20T19:51:04Z

go/vt/schemamanager/schemamanager_test.go

@@ -282,6 +284,7 @@ func (client *fakeTabletManagerClient) ExecuteFetchAsDba(ctx context.Context, ta
 return client.TabletManagerClient.ExecuteFetchAsDba(ctx, tablet, usePool, query, maxRows, disableBinlogs, reloadSchema)
 }

+// FIXME(alainjobart) replace with a memorytopo.


Why you did this in some tests and didn't do in others?

I thought this would be too much for a single PR, but I guess it's huge already.

Pushed a commit with a fix for this, working on the rest now.

pivanof · 2017-11-20T20:00:10Z

go/vt/topo/helpers/copy.go

@@ -80,7 +80,9 @@ func CopyKeyspaces(ctx context.Context, fromTS, toTS topo.Impl) {

 // CopyShards will create the shards in the destination topo.
 func CopyShards(ctx context.Context, fromTS, toTS topo.Impl) {


Why you didn't make all functions to accept topo.Server instead of topo.Impl?

pivanof · 2017-11-20T21:12:37Z

go/vt/topo/replication.go

+
+// UpdateShardReplicationFields updates the fields inside a topo.ShardReplication object.
+func (ts Server) UpdateShardReplicationFields(ctx context.Context, cell, keyspace, shard string, update func(*topodatapb.ShardReplication) error) error {
+ nodePath := path.Join(KeyspacesPath, keyspace, ShardsPath, shard, ShardReplicationFile)


Shouldn't you convert the shard name to lowercase in all these functions?

I don't think we need to do that any more, it was a leftover from old code.

You mean now all the code everywhere uses only shard names in lower case and there is no possibility to pass shard names through a flag (or read it from topology)?

You can pass shard names through a flag, but if you use the wrong case, it will just fail (with unknown shard error). Same as for keyspaces, if you use the wrong case, it will fail. That's just more consistent.

But in most cases, we go through ValidateShard (see above) which already does the ToLower. So all storage is already lower case.

A bit more on this: all code path that alter the topo go through ValidateShard, or TabletComplete, which will both fix the shard name to lower case for keyranges. So we really guarantee everything is lower case for keyranges (like a0-b0). For non-keyranges, we will just keep the case, for all topo implementations alike.

pivanof · 2017-11-20T21:30:17Z

go/vt/topo/srv_keyspace.go

+
+// UpdateSrvKeyspace saves a new SrvKeyspace. It is a blind write.
+func (ts Server) UpdateSrvKeyspace(ctx context.Context, cell, keyspace string, srvKeyspace *topodatapb.SrvKeyspace) error {
+ nodePath := srvKeyspaceFileName(keyspace)


Should you replace "keyspaces" in srvKeyspaceFileName above with KeyspacesPath for consistency?

pivanof · 2017-11-20T21:40:36Z

go/vt/topo/test/keyspace.go

@@ -91,44 +91,30 @@ func checkKeyspace(t *testing.T, ts topo.Impl) {
 t.Errorf("GetKeyspaces: want %v, got %v", []string{"test_keyspace", "test_keyspace2"}, keyspaces)
 }

- // re-read and update.
- storedK, storedVersion, err := ts.GetKeyspace(ctx, "test_keyspace2")
+ // Re-read and update. Have to lock it.


Why have to lock it? Without the explanation the comment is not very useful.

UpdateKeyspace checks for the lock. Updated comment.

pivanof · 2017-11-20T21:43:14Z

go/vt/topo/test/keyspace.go

-
- // unconditional update
- storedK.ShardingColumnType = topodatapb.KeyspaceIdType_BYTES
- _, err = ts.UpdateKeyspace(ctx, "test_keyspace2", storedK, -1)


There was one more update here. Why did you remove it?

We don't do keyspace unconditional updates, so I removed the test case for it. The new API doesn't surface version=nil here.

pivanof · 2017-11-20T21:44:33Z

go/vt/topo/test/lock.go

@@ -34,7 +34,9 @@ var timeUntilLockIsTaken = 10 * time.Millisecond
 // checkKeyspaceLock checks we can take a keyspace lock as expected.
 func checkKeyspaceLock(t *testing.T, ts topo.Impl) {
 ctx := context.Background()
- if err := ts.CreateKeyspace(ctx, "test_keyspace", &topodatapb.Keyspace{}); err != nil {
+ // FIXME(alainjobart) when everywhere, just pass topo.Server as ts.


Why it's not everywhere yet?

Next on my list is to push the Lock API down to the topo.Backend interface. Will be a different PR though, this one is big already. But coming up next.

pivanof · 2017-11-20T22:00:49Z

go/vt/topo/test/vschema.go

- Name: "stln1",
- },
- },
- },


Why did you remove the vschema definition here?

The new higher level API checks it is correct, and this one wasn't.

pivanof · 2017-11-20T22:07:05Z

go/vt/vtctld/explorer.go

+}
+
+// Result is what the backendExplorer returns. It represents one directory node.
+// It is exported to the JSON encoder can print it.


This sentence has two verbs.

Less hacking of the topo.Impl that way. Also checking for error text in errors, so we know we're catching the right error.

Main thing is the consistent use of topo.Server in topo/helpers.

alainjobart

Addressed all comments, PTAL.
Probably easier to just look at the last two commits.

alainjobart · 2017-11-21T16:39:56Z

go/vt/topo/helpers/copy.go

@@ -80,7 +80,9 @@ func CopyKeyspaces(ctx context.Context, fromTS, toTS topo.Impl) {

 // CopyShards will create the shards in the destination topo.
 func CopyShards(ctx context.Context, fromTS, toTS topo.Impl) {


alainjobart · 2017-11-21T16:42:24Z

go/vt/topo/replication.go

+
+// UpdateShardReplicationFields updates the fields inside a topo.ShardReplication object.
+func (ts Server) UpdateShardReplicationFields(ctx context.Context, cell, keyspace, shard string, update func(*topodatapb.ShardReplication) error) error {
+ nodePath := path.Join(KeyspacesPath, keyspace, ShardsPath, shard, ShardReplicationFile)


I don't think we need to do that any more, it was a leftover from old code.

alainjobart · 2017-11-21T16:43:13Z

go/vt/topo/srv_keyspace.go

+
+// UpdateSrvKeyspace saves a new SrvKeyspace. It is a blind write.
+func (ts Server) UpdateSrvKeyspace(ctx context.Context, cell, keyspace string, srvKeyspace *topodatapb.SrvKeyspace) error {
+ nodePath := srvKeyspaceFileName(keyspace)


alainjobart · 2017-11-21T16:44:26Z

go/vt/topo/test/keyspace.go

@@ -91,44 +91,30 @@ func checkKeyspace(t *testing.T, ts topo.Impl) {
 t.Errorf("GetKeyspaces: want %v, got %v", []string{"test_keyspace", "test_keyspace2"}, keyspaces)
 }

- // re-read and update.
- storedK, storedVersion, err := ts.GetKeyspace(ctx, "test_keyspace2")
+ // Re-read and update. Have to lock it.


UpdateKeyspace checks for the lock. Updated comment.

alainjobart · 2017-11-21T16:45:05Z

go/vt/topo/test/keyspace.go

-
- // unconditional update
- storedK.ShardingColumnType = topodatapb.KeyspaceIdType_BYTES
- _, err = ts.UpdateKeyspace(ctx, "test_keyspace2", storedK, -1)


We don't do keyspace unconditional updates, so I removed the test case for it. The new API doesn't surface version=nil here.

alainjobart · 2017-11-21T16:46:04Z

go/vt/topo/test/lock.go

@@ -34,7 +34,9 @@ var timeUntilLockIsTaken = 10 * time.Millisecond
 // checkKeyspaceLock checks we can take a keyspace lock as expected.
 func checkKeyspaceLock(t *testing.T, ts topo.Impl) {
 ctx := context.Background()
- if err := ts.CreateKeyspace(ctx, "test_keyspace", &topodatapb.Keyspace{}); err != nil {
+ // FIXME(alainjobart) when everywhere, just pass topo.Server as ts.


Next on my list is to push the Lock API down to the topo.Backend interface. Will be a different PR though, this one is big already. But coming up next.

alainjobart · 2017-11-21T16:46:28Z

go/vt/topo/test/vschema.go

- Name: "stln1",
- },
- },
- },


The new higher level API checks it is correct, and this one wasn't.

alainjobart · 2017-11-21T16:46:37Z

go/vt/vtctld/explorer.go

+}
+
+// Result is what the backendExplorer returns. It represents one directory node.
+// It is exported to the JSON encoder can print it.


alainjobart · 2017-11-21T17:19:35Z

And thanks @pivanof for the review, this is a lot of cleanup. 👍

pivanof · 2017-11-21T20:49:45Z

go/vt/schemamanager/tablet_executor_test.go

+ }
+ // This will create the Keyspace, Shard and Tablet record.
+ // Since this is a replica tablet, the Shard will have no master.
+ if err := wr.InitTablet(ctx, tablet /*allowMasterOverride=*/, false /*createShardAndKeyspace=*/, true /*allowUpdate*/, false); err != nil {


The commas are on the wrong side of comments here. They should be on the left, because the comment is related to the value on the right.

I always get hit by this, gofmt has different format that C. Fixed.

pivanof · 2017-11-21T20:52:53Z

go/vt/topo/helpers/tee_test.go

- // create a tee and check it implements the interface
- tee := NewTee(fromTS, toTS, true)
+ // create a tee and check it implements the interface.
+ tee := NewTee(fromTS.Impl, toTS.Impl, true)


Should actually NewTee accept Server instead of Impl and return Server as well?

pivanof · 2017-11-21T21:03:15Z

go/vt/topo/replication.go

+
+// UpdateShardReplicationFields updates the fields inside a topo.ShardReplication object.
+func (ts Server) UpdateShardReplicationFields(ctx context.Context, cell, keyspace, shard string, update func(*topodatapb.ShardReplication) error) error {
+ nodePath := path.Join(KeyspacesPath, keyspace, ShardsPath, shard, ShardReplicationFile)


You mean now all the code everywhere uses only shard names in lower case and there is no possibility to pass shard names through a flag (or read it from topology)?

pivanof · 2017-11-21T21:08:15Z

go/vt/topo/test/replication.go

@@ -27,9 +27,10 @@ import (

 // checkShardReplication tests ShardReplication objects
 func checkShardReplication(t *testing.T, ts topo.Impl) {


It feels like this function needs to accept Server instead of Impl as well.

alainjobart · 2017-11-21T21:40:04Z

@pivanof addressed latest comments, with commit.

One comment was lost in there, but I saw it somewhere:

Should actually NewTee accept Server instead of Impl and return Server as well?

I think so, but I also have on my list to refactor some of that code, so I'll do it then, if you don't mind. Right now, the tee is at the Impl level, it should either be at Backend, or Server, as Impl is going to disappear. So this code will need to change at some point again.

…client mount command more standard (vitessio#3402) * backport of 3398 * Fix conflict Signed-off-by: Matt Lord <mattalord@gmail.com> --------- Signed-off-by: Matt Lord <mattalord@gmail.com> Co-authored-by: Matt Lord <mattalord@gmail.com>

googlebot added the cla: yes label Nov 17, 2017

alainjobart added 3 commits November 17, 2017 11:12

Adding more tests for the tee.

e6c253e

Just want to make sure the version code works, as it is different for the topo.Backend methods than it was for the *Keyspace methods.

Removing duplicate Shard related code from topos.

35eedcd

And fixing related issues.

Now removing all Tablet duplicate code in topo.

1771fa7

In the process, converting the discovery topology watcher code to use a regular memorytopo, instead of a custom topo.

alainjobart force-pushed the simpletopo branch from fa41e97 to 1771fa7 Compare November 18, 2017 03:51

alainjobart added 4 commits November 19, 2017 10:46

Now factoring out ShardReplication code into topo.

9951d00

Factoring out Serving Graph methods.

4f2c4e3

Now factoring out VSchema for topo.

1416679

Simplified vtctld.Explorer code.

ecb2274

No need to have the interface now that we only have one implementation. Moved everything into vtctld/explorer.go.

alainjobart requested a review from sougou November 19, 2017 21:12

alainjobart changed the title ~~[WIP] Simplifying topo implementations.~~ Simplifying topo implementations. Nov 19, 2017

alainjobart requested a review from pivanof November 20, 2017 19:08

pivanof reviewed Nov 20, 2017

View reviewed changes

alainjobart added 2 commits November 21, 2017 08:26

Converting schemamanager tests to memorytopo.

6d7a366

Less hacking of the topo.Impl that way. Also checking for error text in errors, so we know we're catching the right error.

Addressing review comments for topo changes.

d0e6bf7

Main thing is the consistent use of topo.Server in topo/helpers.

alainjobart commented Nov 21, 2017

View reviewed changes

pivanof reviewed Nov 21, 2017

View reviewed changes

Using topo.Server more widely.

a7a8ee5

pivanof approved these changes Nov 21, 2017

View reviewed changes

alainjobart merged commit e784ac3 into vitessio:master Nov 22, 2017

		@@ -80,7 +80,9 @@ func CopyKeyspaces(ctx context.Context, fromTS, toTS topo.Impl) {

		// CopyShards will create the shards in the destination topo.
		func CopyShards(ctx context.Context, fromTS, toTS topo.Impl) {

		@@ -27,9 +27,10 @@ import (

		// checkShardReplication tests ShardReplication objects
		func checkShardReplication(t *testing.T, ts topo.Impl) {

Simplifying topo implementations. #3402

Simplifying topo implementations. #3402

Conversation

alainjobart commented Nov 17, 2017 • edited Loading

alainjobart commented Nov 19, 2017

alainjobart commented Nov 19, 2017

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

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

alainjobart 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

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

alainjobart commented Nov 21, 2017

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

alainjobart commented Nov 21, 2017

alainjobart commented Nov 17, 2017 •

edited

Loading