VReplication: Add reference-tables to existing materialize workflow #17804
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17804 +/- ##
==========================================
- Coverage 67.97% 67.46% -0.51%
==========================================
Files 1586 1594 +8
Lines 255195 259200 +4005
==========================================
+ Hits 173468 174874 +1406
- Misses 81727 84326 +2599 ☔ View full report in Codecov by Sentry. |
beingnoble03
force-pushed
the
add-tables-materialize
branch
from
ed0f3e9 to
7d4356f
Compare
Signed-off-by: Noble Mittal <[email protected]>
beingnoble03
force-pushed
the
add-tables-materialize
branch
from
7d4356f to
5b317c5
Compare
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
beingnoble03
force-pushed
the
add-tables-materialize
branch
from
7b4c750 to
bdc7309
Compare
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
beingnoble03
added
Type: Enhancement
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
beingnoble03
requested review from
deepthi,
harshit-gangal,
mattlord,
notfelineit,
rohit-nayak-ps and
GuptaManan100
as code owners
|
|
||
| // addReferenceTables makes a MaterializeAddTables gRPC call to a vtctld. | ||
| addReferenceTables = &cobra.Command{ | ||
| Use: "add-reference-tables --tables='table1,table2'", |
There was a problem hiding this comment.
This command name follows flag name conventions. It should be addreferencetables instead.
I wonder why we should limit this to reference tables? I know that was the use case in the feature request but the same underlying issues apply to any materialized table, no? Toward that end, I think it might be nice to have:
materialize --target-keyspace=commerce --workflow=foo update --add-tables 'table1,table2'
Then in the future we could support other updates such as removing tables.
There was a problem hiding this comment.
Sounds good! But, do you think we should also add a TableSettings flag for this?
| sourceKeyspace string | ||
| workflowType binlogdatapb.VReplicationWorkflowType | ||
| ) | ||
|
|
There was a problem hiding this comment.
We need to get a named workflow lock and a target keyspace lock here. Otherwise there's no concurrency control in place and behavior is undefined.
go/vt/vtctl/workflow/server.go
Outdated
| mu.Lock() | ||
| if len(res.Streams) > 0 && sourceKeyspace == "" { | ||
| sourceKeyspace = res.Streams[0].Bls.Keyspace | ||
| } | ||
| workflowType = res.WorkflowType | ||
| readVReplicationWorkflowResp[tablet.Shard] = res | ||
| mu.Unlock() |
There was a problem hiding this comment.
Nit, but IMO it's worth using a closure here to be sure the mutex is unlocked using a defer (e.g. you get a panic between the lock and unlock, and that panic is recovered up the call stack, but then we don't unlock the mutex — although in this specific case it would be fine since this mutex would go out of scope in that case).
func() {
mu.Lock()
defer mu.Unlock()
if len(res.Streams) > 0 && sourceKeyspace == "" {
sourceKeyspace = res.Streams[0].Bls.Keyspace
}
workflowType = res.WorkflowType
readVReplicationWorkflowResp[tablet.Shard] = res
}()
| } | ||
| // We only allow adding tables for MoveTables and Materialize workflows. | ||
| if workflowType != binlogdatapb.VReplicationWorkflowType_Materialize && | ||
| workflowType != binlogdatapb.VReplicationWorkflowType_MoveTables { |
There was a problem hiding this comment.
OK, so we can support adding tables to a MoveTables workflow later too 👍 . It's another reason why I think we should probably use the update sub-command as noted above.
There was a problem hiding this comment.
yeah, MaterializeAddTables() still works for both MoveTables and Materialize, only thing that's missing is the sub-command for the movetables.
go/vt/vtctl/workflow/server.go
Outdated
| return err | ||
| } | ||
|
|
||
| mu.Lock() |
There was a problem hiding this comment.
Same note here about using a closure. I also wonder if it's worth making readVReplicationWorkflowResp an atomic map/pointer.
| Query: []byte(buf.String()), | ||
| }) | ||
| if err != nil { | ||
| return vterrors.Wrapf(err, "failed to insert tables copy state for workflow %s on shard %s/%s", req.Workflow, req.Keyspace, tablet.Shard) |
There was a problem hiding this comment.
Any reason not to include the table name here?
There was a problem hiding this comment.
we are using multiple value insert here, doesn't look like we can point out the specific table. moreover, we are wrapping the error so it can help i think.
Signed-off-by: Noble Mittal <[email protected]>
Description
This PR adds
MaterializeAddTablesinVtctldServer. This can be used to add tables to an existing VReplication MoveTables/Materialize workflow. It adds binlogsource rules in the existing workflow streams, and inserts the copy state row in_vt.copy_stateand restarts the workflow. Also adds aadd-reference-tablessub-command formaterializecommand, which can be used as following:Related Issue(s)
Checklist
Deployment Notes