Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VReplication: Properly ignore errors from trying to drop tables that don't exist #16505

Merged
merged 10 commits into from
Aug 8, 2024
Merged

VReplication: Properly ignore errors from trying to drop tables that don't exist #16505

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a5297e2
Convert grpc status.Error to sqlerror.SQLError
mattlord Jul 30, 2024
feb47bb
Add test
mattlord Jul 30, 2024
b215529
Minor changes from self review
mattlord Jul 30, 2024
f834270
Address review comments
mattlord Jul 30, 2024
91dd55b
Cleanup imports
mattlord Jul 30, 2024
18812c8
Nitty nit
mattlord Jul 30, 2024
7ce084b
Add unit test code for dropping a table that doesn't exist
mattlord Jul 30, 2024
5d9d625
Remove unintentional/extraneous change
mattlord Jul 30, 2024
de0817d
Nits
mattlord Aug 1, 2024
83cbdae
Nits part deux
mattlord Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion go/test/endtoend/vreplication/materialize_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ const smMaterializeSchemaSource = `

const smMaterializeVSchemaSource = `
{
"sharded": true,
"tables": {
"mat": {
"column_vindexes": [
Expand Down Expand Up @@ -197,6 +196,8 @@ func testMaterialize(t *testing.T, useVtctldClient bool) {
_, err = ks2Primary.QueryTablet(customFunc, targetKs, true)
require.NoError(t, err)

testMaterializeWithNonExistentTable(t)

materialize(t, smMaterializeSpec2, useVtctldClient)
catchup(t, ks2Primary, "wf1", "Materialize")

Expand Down
12 changes: 12 additions & 0 deletions go/test/endtoend/vreplication/vreplication_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1183,6 +1183,18 @@ func materialize(t *testing.T, spec string, useVtctldClient bool) {
}
}

func testMaterializeWithNonExistentTable(t *testing.T) {
t.Run("vtctldclient materialize with nonexistent table", func(t *testing.T) {
tableSettings := `[{"target_table": "table_that_doesnt_exist", "create_ddl": "create table mat_val_counts (mat_val varbinary(10), cnt int unsigned, primary key (mat_val))", "source_expression": "select val, count(*) as cnt from mat group by val"}]`
output, err := vc.VtctldClient.ExecuteCommandWithOutput("materialize", "--workflow=tablenogood", "--target-keyspace=source",
"create", "--source-keyspace=source", "--table-settings", tableSettings)
require.NoError(t, err, "Materialize create failed, err: %v, output: %s", err, output)
waitForWorkflowState(t, vc, "source.tablenogood", binlogdatapb.VReplicationWorkflowState_Stopped.String())
output, err = vc.VtctldClient.ExecuteCommandWithOutput("materialize", "--workflow=tablenogood", "--target-keyspace=source", "cancel")
require.NoError(t, err, "Materialize cancel failed, err: %v, output: %s", err, output)
})
}

func materializeProduct(t *testing.T, useVtctldClient bool) {
t.Run("materializeProduct", func(t *testing.T) {
// Materializing from "product" keyspace to "customer" keyspace.
Expand Down
4 changes: 3 additions & 1 deletion go/vt/vtctl/workflow/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2548,7 +2548,9 @@ func (s *Server) optimizeCopyStateTable(tablet *topodatapb.Tablet) {
Query: []byte(sqlOptimizeTable),
MaxRows: uint64(100), // always produces 1+rows with notes and status
}); err != nil {
if sqlErr, ok := err.(*sqlerror.SQLError); ok && sqlErr.Num == sqlerror.ERNoSuchTable { // the table may not exist
// The error is a gRPC status.Error, so we need to convert it to an sqlerror.SQLError.
if mysqlErr, ok := sqlerror.NewSQLErrorFromError(err).(*sqlerror.SQLError); ok &&
(mysqlErr.Num == sqlerror.ERNoSuchTable || mysqlErr.Num == sqlerror.ERBadTable) {
return
}
log.Warningf("Failed to optimize the copy_state table on %q: %v", tablet.Alias.String(), err)
Expand Down
8 changes: 6 additions & 2 deletions go/vt/vtctl/workflow/traffic_switcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,9 @@ func (ts *trafficSwitcher) removeSourceTables(ctx context.Context, removalType T
DisableForeignKeyChecks: true,
})
if err != nil {
if mysqlErr, ok := err.(*sqlerror.SQLError); ok && mysqlErr.Num == sqlerror.ERNoSuchTable {
// The error is a gRPC status.Error, so we need to convert it to an sqlerror.SQLError.
if mysqlErr, ok := sqlerror.NewSQLErrorFromError(err).(*sqlerror.SQLError); ok &&
(mysqlErr.Num == sqlerror.ERNoSuchTable || mysqlErr.Num == sqlerror.ERBadTable) {
ts.Logger().Warningf("%s: Table %s did not exist when attempting to remove it", topoproto.TabletAliasString(source.GetPrimary().GetAlias()), tableName)
return nil
mattlord marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

@mattlord mattlord Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a separate bug fixed here that would have caused the Delete/Cancel to leave any yet to be removed tables orphaned on the source/target. We never got into this block due to the noted issues, however, so this bug could not be triggered before this PR and thus in effect did not exist. The unit test (and framework used) was also updated to test for this.

}
Expand Down Expand Up @@ -1179,7 +1181,9 @@ func (ts *trafficSwitcher) removeTargetTables(ctx context.Context) error {
})
log.Infof("Removed target table with result: %+v", res)
if err != nil {
if mysqlErr, ok := err.(*sqlerror.SQLError); ok && mysqlErr.Num == sqlerror.ERNoSuchTable {
// The error is a gRPC status.Error, so we need to convert it to an sqlerror.SQLError.
mattlord marked this conversation as resolved.
Show resolved Hide resolved
if mysqlErr, ok := sqlerror.NewSQLErrorFromError(err).(*sqlerror.SQLError); ok &&
(mysqlErr.Num == sqlerror.ERNoSuchTable || mysqlErr.Num == sqlerror.ERBadTable) {
// The table was already gone, so we can ignore the error.
ts.Logger().Warningf("%s: Table %s did not exist when attempting to remove it", topoproto.TabletAliasString(target.GetPrimary().GetAlias()), tableName)
return nil
Expand Down
Loading