Materialize: Only get schema from source tablets if target is missing tables #6601
Conversation
… tables Signed-off-by: Rohit Nayak <rohit@planetscale.com>
|
Is this a regression introduced in #6251? |
This one: #6207. I guess you are asking whether this should be backported into 7.0. Not sure technically if it is a regression since we haven't said explicitly that we work with source keyspaces without masters. But it seems to have affected a couple of users already who are using source keyspaces with just replicas. |
Still a regression, I think we should backport. Running with replicas seems like a common usage with unmanaged vttablet mode. |
go/vt/wrangler/materializer.go
Outdated
| } | ||
|
|
||
| targetTablet, err := mz.wr.ts.GetTablet(ctx, target.MasterAlias) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| applyDDLs := []string{} | ||
| var applyDDLs []string | ||
| { |
There was a problem hiding this comment.
why is this an empty clause?
go/vt/wrangler/materializer.go
Outdated
| func (mz *materializer) deploySchema(ctx context.Context) error { | ||
| var tableDDLs map[string]string |
There was a problem hiding this comment.
| var tableDDLs map[string]string | |
| var sourceDDLs map[string]string |
i think it's clearer that it's from the source, hence the name
go/vt/wrangler/materializer.go
Outdated
| @@ -589,51 +590,55 @@ func (wr *Wrangler) buildMaterializer(ctx context.Context, ms *vtctldatapb.Mater | |||
| }, nil | |||
| } | |||
|
|
|||
| func (mz *materializer) getTableDDLs(ctx context.Context) (map[string]string, error) { | |||
There was a problem hiding this comment.
| func (mz *materializer) getTableDDLs(ctx context.Context) (map[string]string, error) { | |
| func (mz *materializer) getSourceTableDDLs(ctx context.Context) (map[string]string, error) { |
go/vt/wrangler/materializer.go
Outdated
| @@ -50,6 +50,7 @@ type materializer struct { | |||
| targetVSchema *vindexes.KeyspaceSchema | |||
| sourceShards []*topo.ShardInfo | |||
| targetShards []*topo.ShardInfo | |||
| mu sync.Mutex | |||
There was a problem hiding this comment.
optional, but wanna mention why this mutex exists? it's not really protecting anything inside the materializer right? i think it's protecting the getting of the getTableDDLs? and is that only an optimization? if so, does it make sense to just have it inside the deploy?
There was a problem hiding this comment.
I think this mutex is needed, but for a different reason, given below.
go/vt/wrangler/materializer.go
Outdated
| @@ -50,6 +50,7 @@ type materializer struct { | |||
| targetVSchema *vindexes.KeyspaceSchema | |||
| sourceShards []*topo.ShardInfo | |||
| targetShards []*topo.ShardInfo | |||
| mu sync.Mutex | |||
There was a problem hiding this comment.
I think this mutex is needed, but for a different reason, given below.
go/vt/wrangler/materializer.go
Outdated
| @@ -589,51 +590,55 @@ func (wr *Wrangler) buildMaterializer(ctx context.Context, ms *vtctldatapb.Mater | |||
| }, nil | |||
| } | |||
|
|
|||
| func (mz *materializer) getTableDDLs(ctx context.Context) (map[string]string, error) { | |||
There was a problem hiding this comment.
This function should run only once per invocation, and all others should use the result: it should populate a new tableDDLs member of mz. Right now, it returns the value to the calling goroutine, which will cause it to execute for each one of those.
Using sync.Once would have been ideal, but you have to handle the error return. So, you may have to live with the global mu. So, it may be better to rename it to schemaMu.
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
During materialize we sometimes need to copy the schema from source to target, depending on the value of the CreateDDL setting. A recent change introduced a regression where we request schema from sources even if we don't need the schema
to be copied.
This has a side-effect that the source needs to have a master (since we only query masters for schemas). Some users setup source keyspaces only with a replica for the purpose of vreplication into a target keyspace. In these cases the vtctl Materialize
command fails.
This PR requests the schema lazily, only if the target is missing tables.
Signed-off-by: Rohit Nayak rohit@planetscale.com