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: Fix workflow update changed handling #15621

Merged
merged 3 commits into from
Apr 3, 2024
Merged

VReplication: Fix workflow update changed handling #15621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8074898
Fix workflow update changed handling
mattlord Apr 3, 2024
3dd0e13
Several minor changes...
mattlord Apr 3, 2024
a57df52
Add TODO to proto message comments
mattlord Apr 3, 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
1 change: 0 additions & 1 deletion go/cmd/vtctldclient/command/vreplication/workflow/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ func commandUpdateState(cmd *cobra.Command, args []string) error {
TabletTypes: []topodatapb.TabletType{topodatapb.TabletType(textutil.SimulatedNullInt)},
OnDdl: binlogdatapb.OnDDLAction(textutil.SimulatedNullInt),
State: state,
Shards: baseOptions.Shards,
},
}

Expand Down
2 changes: 1 addition & 1 deletion go/cmd/vtctldclient/command/vreplication/workflow/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func commandUpdate(cmd *cobra.Command, args []string) error {
TabletTypes: updateOptions.TabletTypes,
TabletSelectionPreference: tsp,
OnDdl: binlogdatapb.OnDDLAction(onddl),
Shards: baseOptions.Shards,
State: binlogdatapb.VReplicationWorkflowState(textutil.SimulatedNullInt), // We don't allow changing this in the client command
Copy link
Member

Choose a reason for hiding this comment

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

If we're not using the cli, is there documentation for when the zero value isn't sufficient for a no-op? Does it make sense to at least add field level comments to the proto?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I've added comments here: 3dd0e13

Copy link
Member

Choose a reason for hiding this comment

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

That's helpful. Is it backwards compatible if we add optional to those fields?

Copy link
Contributor Author

@mattlord mattlord Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the go protobuf implementation everything is optional. And unfortunately it does not support the more recent optional protobuf behavior where code is then injected to detect if a value was explicitly provided in the message. That's what the simulated NULL stuff is all about, because there's literally no way to determine if a value was actually provided or not (type's ZeroValue). I'm not terribly happy with it, but I could not find a less bad option today. I'd love to get rid of the simulated NULL whenever a better option appears.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, my understanding is now dated! The go implementation DOES now support truly optional field behavior: https://github.com/protocolbuffers/protobuf/blob/v3.25.0/docs/field_presence.md

It does that by making the specified type, e.g. string, a pointer (*string). I'm looking into whether or not that's backward compatible now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#15627 ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a TODO in the comments as well to link everything together.

},
}

Expand Down
1 change: 1 addition & 0 deletions go/test/endtoend/vreplication/multi_tenant_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ func TestMultiTenantSimple(t *testing.T) {
for _, ks := range []string{sourceKeyspace, sourceAliasKeyspace} {
lastIndex = insertRows(lastIndex, ks)
}
waitForWorkflowState(t, vc, fmt.Sprintf("%s.%s", targetKeyspace, mt.workflowName), binlogdatapb.VReplicationWorkflowState_Running.String())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is unrelated, but addresses a test flake that was observed on the PR CI runs.

mt.SwitchReadsAndWrites()
validateKeyspaceRoutingRules(t, vc, primaries, rulesMap, true)
// Note: here we have already switched and we can insert into the target keyspace and it should get reverse
Expand Down
31 changes: 28 additions & 3 deletions go/test/endtoend/vreplication/resharding_workflows_v2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,14 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/encoding/protojson"

"vitess.io/vitess/go/test/endtoend/cluster"
"vitess.io/vitess/go/vt/log"
"vitess.io/vitess/go/vt/wrangler"

binlogdatapb "vitess.io/vitess/go/vt/proto/binlogdata"
vtctldatapb "vitess.io/vitess/go/vt/proto/vtctldata"
)

const (
Expand Down Expand Up @@ -212,19 +214,42 @@ func tstWorkflowComplete(t *testing.T) error {
// to primary,replica,rdonly (the only applicable types in these tests).
func testWorkflowUpdate(t *testing.T) {
tabletTypes := "primary,replica,rdonly"
// Test vtctlclient first
// Test vtctlclient first.
_, err := vc.VtctlClient.ExecuteCommandWithOutput("workflow", "--", "--tablet-types", tabletTypes, "noexist.noexist", "update")
require.Error(t, err, err)
resp, err := vc.VtctlClient.ExecuteCommandWithOutput("workflow", "--", "--tablet-types", tabletTypes, ksWorkflow, "update")
require.NoError(t, err)
require.NotEmpty(t, resp)

// Test vtctldclient last
// Test vtctldclient last.
_, err = vc.VtctldClient.ExecuteCommandWithOutput("workflow", "--keyspace", "noexist", "update", "--workflow", "noexist", "--tablet-types", tabletTypes)
require.Error(t, err)
// Change the tablet-types to rdonly.
resp, err = vc.VtctldClient.ExecuteCommandWithOutput("workflow", "--keyspace", targetKs, "update", "--workflow", workflowName, "--tablet-types", "rdonly")
require.NoError(t, err, err)
// Confirm that we changed the workflow.
var ures vtctldatapb.WorkflowUpdateResponse
require.NoError(t, err)
err = protojson.Unmarshal([]byte(resp), &ures)
require.NoError(t, err)
require.Greater(t, len(ures.Details), 0)
require.True(t, ures.Details[0].Changed)
// Change tablet-types back to primary,replica,rdonly.
resp, err = vc.VtctldClient.ExecuteCommandWithOutput("workflow", "--keyspace", targetKs, "update", "--workflow", workflowName, "--tablet-types", tabletTypes)
require.NoError(t, err, err)
require.NotEmpty(t, resp)
// Confirm that we changed the workflow.
err = protojson.Unmarshal([]byte(resp), &ures)
require.NoError(t, err)
require.Greater(t, len(ures.Details), 0)
require.True(t, ures.Details[0].Changed)
// Execute a no-op as tablet-types is already primary,replica,rdonly.
resp, err = vc.VtctldClient.ExecuteCommandWithOutput("workflow", "--keyspace", targetKs, "update", "--workflow", workflowName, "--tablet-types", tabletTypes)
require.NoError(t, err, err)
// Confirm that we didn't change the workflow.
err = protojson.Unmarshal([]byte(resp), &ures)
require.NoError(t, err)
require.Greater(t, len(ures.Details), 0)
require.False(t, ures.Details[0].Changed)
}

func tstWorkflowCancel(t *testing.T) error {
Expand Down
139 changes: 71 additions & 68 deletions go/vt/proto/tabletmanagerdata/tabletmanagerdata.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading