[vtctldserver] OnlineDDL protobuf message introduction #12195

ajm188 · 2023-01-31T23:38:45Z

Description

Introducing a formal protobuf type for OnlineDDL types to use across the existing onlineddl code, vtctldserver (and client), and eventually vtadmin.

This is a prerequisite to moving any of the command functionality, as it will help us ensure we're maintaining compatibility if both the new and old code is using the same underlying types.

In order to do this I needed to slightly change some of the field names, which I've (attempted) to do in a backwards-compatible way, issuing deprecation warnings anywhere we parse in an "old" way.

Related Issue(s)

Closes #13713

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Documentation was added or is not required

Deployment Notes

vitess-bot · 2023-01-31T23:38:48Z

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

Ensure that the Pull Request has a descriptive title.
If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
If a test is added or modified, there should be a documentation on top of the test to explain what the expected behavior is what the test does.

If a new flag is being introduced:

Is it really necessary to add this flag?
Flag names should be clear and intuitive (as far as possible)
Help text should be descriptive.
Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

Each item in Jobs should be named in order to mark it as required.
If the workflow should be required, the maintainer team should be notified.

Bug fixes

There should be at least one unit or end-to-end test.
The Pull Request description should include a link to an issue that describes the bug.

Non-trivial changes

There should be some code comments as to why things are implemented the way they are.

New/Existing features

Should be documented, either by modifying the existing documentation or creating new documentation.
New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

Protobuf changes should be wire-compatible.
Changes to _vt tables and RPCs need to be backward compatible.
vtctl command output order should be stable and awk-able.
RPC changes should be compatible with vitess-operator
If a flag is removed, then it should also be removed from VTop, if used there.

github-actions · 2023-03-04T01:32:53Z

This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:

Push additional commits to the associated branch.
Remove the stale label.
Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

github-actions · 2023-04-06T01:36:12Z

This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:

Push additional commits to the associated branch.
Remove the stale label.
Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

github-actions · 2023-05-10T01:31:45Z

This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:

Push additional commits to the associated branch.
Remove the stale label.
Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

github-actions · 2023-05-17T01:32:50Z

This PR was closed because it has been stale for 7 days with no activity.

Signed-off-by: Andrew Mason <[email protected]>

…rds compat Signed-off-by: Andrew Mason <[email protected]>

Signed-off-by: Andrew Mason <[email protected]>

shlomi-noach

@ajm188 wow, what a massive effort! 🙇 I've left several comments throughout the PR. But, I would also like to carefully question a basic premise here. I'm not in on the many intricates of vtctld and you would know better than me; also, you seem to already have a plan for OnlineDDL commands which we haven't discussed, so I definitely might just miss something.

This change is so massive first and foremost due to moving OnlineDDL to proto. The question being: why do we need OnlineDDL to be in proto? While I don't have a clear grasp of how I'd like to see vtctldclient OnlineDDL commands implemented, I am not sure I see the need for OnlineDDL to pass as a message, in either direction. All I think is needed are a command ("cancel", "complete", "show") and a UUID/"all", for most cases. I don't see that the state of an OnlineDDL struct should pass back or forth?

Basically, the entire existence of the OnlineDDL command was made as a convenience. Most of the commands can be applied via ApplySchema anyway. So it's mostly syntactic sugar. It was created at a time where we envisioned VExec to be a new mechanism with which to control all-things-internal, but we've moved away from that idea. But the command vtctl OnlineDDL caught on, being useful, and so we want to keep supporting it -- however I'd hate for this otherwise syntactic-sugar command to cause such a disruption in the code/design. In my comments below I raise a few concerns over some of the changes.

I'm happy to discuss further. This is such a tremendous work 👏 and again I might miss something very important, but I also want to be cautious and make sure we don't make changes that aren't strictly necessary 🙏

shlomi-noach · 2023-08-06T06:06:02Z

go/vt/schema/internal/hooks/decode.go

Can you please explain what the functions in this file do? I'm a bit lost.

shlomi-noach · 2023-08-06T06:07:32Z

go/cmd/vtctldclient/command/schema.go

@@ -287,7 +287,7 @@ func commandReloadSchemaShard(cmd *cobra.Command, args []string) error {
 func init() {
 	ApplySchema.Flags().MarkDeprecated("--allow-long-unavailability", "")
 	ApplySchema.Flags().MarkDeprecated("--skip-preflight", "Deprecated. Assumed to be always 'true'")
-	ApplySchema.Flags().StringVar(&applySchemaOptions.DDLStrategy, "ddl-strategy", string(schema.DDLStrategyDirect), "Online DDL strategy, compatible with @@ddl_strategy session variable (examples: 'gh-ost', 'pt-osc', 'gh-ost --max-load=Threads_running=100'.")
+	ApplySchema.Flags().StringVar(&applySchemaOptions.DDLStrategy, "ddl-strategy", strings.ToLower(tabletmanagerdatapb.OnlineDDL_Strategy_name[int32(tabletmanagerdatapb.OnlineDDL_DIRECT)]), "Online DDL strategy, compatible with @@ddl_strategy session variable (examples: 'gh-ost', 'pt-osc', 'gh-ost --max-load=Threads_running=100'.")


Can we have a helper function to simplify strings.ToLower(tabletmanagerdatapb.OnlineDDL_Strategy_name[int32(...)])? This kind of logic recurs a few times in the code and I find it difficult to parse.

shlomi-noach · 2023-08-06T06:09:00Z

go/test/endtoend/onlineddl/ghost/onlineddl_ghost_test.go

@@ -221,11 +223,11 @@ func TestSchemaChange(t *testing.T) {

 	testWithInitialSchema(t)
 	t.Run("create non_online", func(t *testing.T) {
-		_ = testOnlineDDLStatement(t, alterTableNormalStatement, string(schema.DDLStrategyDirect), "vtctl", "non_online", "")
+		_ = testOnlineDDLStatement(t, alterTableNormalStatement, "direct", "vtctl", "non_online", "")


Can we have a helper function that translates OnlineDDL_DIRECT to the string "direct"?

Presumably based on tabletmanagerdatapb.OnlineDDL_Strategy_name[int32(strategy)]?

shlomi-noach · 2023-08-06T06:15:02Z

go/vt/schema/ddl_strategy.go

+		return tabletmanagerdatapb.OnlineDDL_DIRECT, nil
+	}
+
+	lowerName := strings.ToUpper(name)


lowerName := strings.ToUpper(...) is contradictory is confusing. Let's do use ToLower() and look for texts like "gh-ost" instread of "GH-OST"

Suggested change

lowerName := strings.ToUpper(name)

lowerName := strings.ToLower(name)

ah it should actually be upperName because we need the ALL_CAPS to index into the enum table

shlomi-noach · 2023-08-06T06:18:29Z

go/vt/schema/ddl_strategy_test.go

+		var assertion func(t assert.TestingT, value bool, msgAndArgs ...any) bool
+		switch direct {
+		case true:
+			assertion = assert.True
+		case false:
+			assertion = assert.False
+		}
+
+		assertion(t, NewDDLStrategySetting(strategy, "").IsDirect())


I think this reads simpler:

Suggested change

var assertion func(t assert.TestingT, value bool, msgAndArgs ...any) bool

switch direct {

case true:

assertion = assert.True

case false:

assertion = assert.False

}

assertion(t, NewDDLStrategySetting(strategy, "").IsDirect())

assert.Equal(t, direct, NewDDLStrategySetting(strategy, "").IsDirect())

shlomi-noach · 2023-08-06T06:22:01Z

go/vt/schema/online_ddl.go

+	// should use the IsReadyToComplete and MarkReadyToComplete methods to
+	// safely modify the value of that field rather than accessing it through
+	// the embedded protobuf message directly.
+	m sync.Mutex


I'm wary of adding a mutex member of OnlineDDL. Throughout the existing code we pass OnlineDDL by reference and by value, and this does not play well with mutex members as their behavior can be inconsistent. OnlineDDL was designed to be a data-only object. I'd say we can perhaps enforce users to use sync/atomic Bool or similar?

shlomi-noach · 2023-08-06T06:23:22Z

go/vt/schema/online_ddl.go

-	onlineDDL := &OnlineDDL{}
-	err := json.Unmarshal(bytes, onlineDDL)
-	return onlineDDL, err
+func FromJSON(data []byte) (*OnlineDDL, error) {


Will it not be safer to create a temporary struct in the likeness of the old OnlineDDL struct, read the JSON into that struct, then copy over the results to the "new" OnlineDDL fields?

shlomi-noach · 2023-08-06T06:24:32Z

go/vt/schema/online_ddl.go

+	onlineDDL.m.Lock()
+	defer onlineDDL.m.Unlock()
+
+	return onlineDDL.ReadyToComplete, onlineDDL.OnlineDDL.WasReadyToComplete


Can we use sync/atomic here instead of using a Mutex?

shlomi-noach · 2023-08-06T06:28:40Z

go/vt/schema/online_ddl.go

+	// safely modify the value of that field rather than accessing it through
+	// the embedded protobuf message directly.
+	m sync.Mutex
+	*tabletmanagerdatapb.OnlineDDL


Should we use tabletmanagerdatapb.OnlineDDL by value and not by pointer? This will be more inline with the current usage. Again, I'm concerned about copying OnlineDDL values: with *tabletmanagerdatapb.OnlineDDL we risk two instances of OnlineDDL sharing data. Maybe there's nothing in the code to worry about, and if all the tests pass then I guess everything's fine. But the original design was to share nothing, and I'm concerned we might miss a leak.

shlomi-noach

@ajm188 wow, what a massive effort! 🙇 I've left several comments throughout the PR. But, I would also like to carefully question a basic premise here. I'm not in on the many intricates of vtctld and you would know better than me; also, you seem to already have a plan for OnlineDDL commands which we haven't discussed, so I definitely might just miss something.

This change is so massive first and foremost due to moving OnlineDDL to proto. The question being: why do we need OnlineDDL to be in proto? While I don't have a clear grasp of how I'd like to see vtctldclient OnlineDDL commands implemented, I am not sure I see the need for OnlineDDL to pass as a message, in either direction. All I think is needed are a command ("cancel", "complete", "show") and a UUID/"all", for most cases. I don't see that the state of an OnlineDDL struct should pass back or forth?

Basically, the entire existence of the OnlineDDL command was made as a convenience. Most of the commands can be applied via ApplySchema anyway. So it's mostly syntactic sugar. It was created at a time where we envisioned VExec to be a new mechanism with which to control all-things-internal, but we've moved away from that idea. But the command vtctl OnlineDDL caught on, being useful, and so we want to keep supporting it -- however I'd hate for this otherwise syntactic-sugar command to cause such a disruption in the code/design. In my comments below I raise a few concerns over some of the changes.

I'm happy to discuss further. This is such a tremendous work 👏 and again I might miss something very important, but I also want to be cautious and make sure we don't make changes that aren't strictly necessary 🙏

shlomi-noach · 2023-08-06T07:09:58Z

(No idea why the review comment submitted twice).

Just a bit more context: the OnlineDDL object is used internally by the tablet, specifically by the onlineddl/Executor to manage the state of migrations. It's only ever used within go/vt/vttablet/onlineddl/.... Externally, there are commands to manipulate migrations, but then those just hand off the UUID of a migration and the keyspace. These commands do not understand the meaning of OnlineDDL's structure. For those commands, IMHO there is no advantage to having a proto for OnlineDDL.

I think the closes to exposing OnlineDDL is SHOW VITESS_MIGRATIONS that runs a SELECT * FROM _vt.schema_migrations. Some of the columns returned by this query, map into an OnlineDDL struct. But by far not all of them. The majority of columns are not represented as fields in OnlineDDL. Hence, the command SHOW VITESS_MIGRATIONS (or OnlineDDL show) needs to somehow return all those columns anyway; having some of them in a proto-ed OnlineDDL doesn't solve that need and so there is IMHO no advantage to having a proto for OnlineDDL for this command, either.

ajm188 · 2023-08-07T17:16:05Z

@shlomi-noach yep, you are exactly correct (regarding "wait, hang on, do we need this?") and i came to the same realization yesterday when I started on onlineddl show. What we actually need is a protobuf representation of the _vt.schema_migrations table, and the only things that are actually useful in this PR are the status/strategy enums and parsers, which don't even need to be defined within tabletmanagerdata.

Wish I had gone with the whole-hog approach rather than this way as I would have realized that sooner, but c'est la vie!

I've got the protobuf working in another branch which I'll open shortly, I have some open questions about exactly how much we want to be showing

ajm188 · 2023-08-08T14:47:36Z

we don't need this, instead see #13738

vitess-bot bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says labels Jan 31, 2023

ajm188 force-pushed the andrew/vtctldserver-onlineddl-protobuf branch from d90ffd8 to c05f69f Compare February 1, 2023 19:31

github-actions bot added the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Mar 4, 2023

ajm188 removed the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Mar 6, 2023

github-actions bot added the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Apr 6, 2023

ajm188 removed the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Apr 9, 2023

github-actions bot added the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label May 10, 2023

github-actions bot closed this May 17, 2023

Andrew Mason added 16 commits August 1, 2023 06:21

add protobuf message for OnlineDDL structure

f019a18

Signed-off-by: Andrew Mason <[email protected]>

embed protobuf in Go structure

6a54395

Signed-off-by: Andrew Mason <[email protected]>

begin moving duplicate fields out of the embedding struct

807d5c4

Signed-off-by: Andrew Mason <[email protected]>

move and rename SQL->Sql

804e7eb

Signed-off-by: Andrew Mason <[email protected]>

add FromJSON test cases

1307cc0

Signed-off-by: Andrew Mason <[email protected]>

move and rename UUID->Uuid

a02d5d2

Signed-off-by: Andrew Mason <[email protected]>

migrate Strategy field from outer structure

d2c41c9

Signed-off-by: Andrew Mason <[email protected]>

fix json parsing, add test cases

9b81a29

Signed-off-by: Andrew Mason <[email protected]>

remove legacy DDLStrategy enum type

abead9a

Signed-off-by: Andrew Mason <[email protected]>

move Options field out of wrapping struct

ce4c867

Signed-off-by: Andrew Mason <[email protected]>

Move Retries field

66129bf

Signed-off-by: Andrew Mason <[email protected]>

wip

90d814f

Signed-off-by: Andrew Mason <[email protected]>

switch to a two-step mapstructure approach to properly support backwa…

bf17773

…rds compat Signed-off-by: Andrew Mason <[email protected]>

migrate Status field out of wrapping structure

dbddd94

Signed-off-by: Andrew Mason <[email protected]>

go mod tidy and small fix

85de1ce

Signed-off-by: Andrew Mason <[email protected]>

extend parsing to include protobuf RequestTime

1eeb4cf

Signed-off-by: Andrew Mason <[email protected]>

Andrew Mason added 4 commits August 2, 2023 19:59

fix decode hook tests

e423990

Signed-off-by: Andrew Mason <[email protected]>

remove e2e suites accidentally re-introduced

7a0e9dd

Signed-off-by: Andrew Mason <[email protected]>

remove RequestTime, add WasReadyToComplete

e22973c

Signed-off-by: Andrew Mason <[email protected]>

finish up deprecation warnings

26726cf

Signed-off-by: Andrew Mason <[email protected]>

ajm188 added Type: Feature Component: Online DDL Online DDL (vitess/native/gh-ost/pt-osc) and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says labels Aug 3, 2023

ajm188 marked this pull request as ready for review August 3, 2023 20:17

ajm188 requested review from deepthi, harshit-gangal, mattlord, systay, shlomi-noach, rohit-nayak-ps, frouioui, GuptaManan100 and notfelineit as code owners August 3, 2023 20:17

ajm188 changed the title ~~[vtctldserver] WIP: onlineddl protobuf~~ [vtctldserver] OnlineDDL protobuf message introduction Aug 4, 2023

shlomi-noach reviewed Aug 6, 2023

View reviewed changes

shlomi-noach mentioned this pull request Aug 6, 2023

Define protobuf message for OnlineDDL messages #13713

Closed

frouioui added this to the v18.0.0 milestone Aug 7, 2023

ajm188 closed this Aug 8, 2023

ajm188 deleted the andrew/vtctldserver-onlineddl-protobuf branch August 8, 2023 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vtctldserver] OnlineDDL protobuf message introduction #12195

[vtctldserver] OnlineDDL protobuf message introduction #12195

ajm188 commented Jan 31, 2023 •

edited

Loading

vitess-bot bot commented Jan 31, 2023

github-actions bot commented Mar 4, 2023

github-actions bot commented Apr 6, 2023

github-actions bot commented May 10, 2023

github-actions bot commented May 17, 2023

shlomi-noach left a comment

shlomi-noach Aug 6, 2023

shlomi-noach Aug 6, 2023

shlomi-noach Aug 6, 2023

shlomi-noach Aug 6, 2023

shlomi-noach Aug 6, 2023

ajm188 Aug 7, 2023

shlomi-noach Aug 6, 2023

shlomi-noach Aug 6, 2023

shlomi-noach Aug 6, 2023

shlomi-noach Aug 6, 2023

shlomi-noach Aug 6, 2023

shlomi-noach left a comment

shlomi-noach commented Aug 6, 2023

ajm188 commented Aug 7, 2023

ajm188 commented Aug 8, 2023

	lowerName := strings.ToUpper(name)
	lowerName := strings.ToLower(name)

[vtctldserver] OnlineDDL protobuf message introduction #12195

[vtctldserver] OnlineDDL protobuf message introduction #12195

Conversation

ajm188 commented Jan 31, 2023 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jan 31, 2023

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

github-actions bot commented Mar 4, 2023

github-actions bot commented Apr 6, 2023

github-actions bot commented May 10, 2023

github-actions bot commented May 17, 2023

shlomi-noach 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomi-noach left a comment

Choose a reason for hiding this comment

shlomi-noach commented Aug 6, 2023

ajm188 commented Aug 7, 2023

ajm188 commented Aug 8, 2023

ajm188 commented Jan 31, 2023 •

edited

Loading