[vtctldserver] OnlineDDL protobuf message introduction by ajm188 · Pull Request #12195 · vitessio/vitess

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

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 <andrew@planetscale.com>

…rds compat Signed-off-by: Andrew Mason <andrew@planetscale.com>

Signed-off-by: Andrew Mason <andrew@planetscale.com>

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

 	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

 	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 <andrew@planetscale.com>

embed protobuf in Go structure

6a54395

Signed-off-by: Andrew Mason <andrew@planetscale.com>

begin moving duplicate fields out of the embedding struct

807d5c4

Signed-off-by: Andrew Mason <andrew@planetscale.com>

move and rename SQL->Sql

804e7eb

Signed-off-by: Andrew Mason <andrew@planetscale.com>

add FromJSON test cases

1307cc0

Signed-off-by: Andrew Mason <andrew@planetscale.com>

move and rename UUID->Uuid

a02d5d2

Signed-off-by: Andrew Mason <andrew@planetscale.com>

migrate Strategy field from outer structure

d2c41c9

Signed-off-by: Andrew Mason <andrew@planetscale.com>

fix json parsing, add test cases

9b81a29

Signed-off-by: Andrew Mason <andrew@planetscale.com>

remove legacy DDLStrategy enum type

abead9a

Signed-off-by: Andrew Mason <andrew@planetscale.com>

move Options field out of wrapping struct

ce4c867

Signed-off-by: Andrew Mason <andrew@planetscale.com>

Move Retries field

66129bf

Signed-off-by: Andrew Mason <andrew@planetscale.com>

wip

90d814f

Signed-off-by: Andrew Mason <andrew@planetscale.com>

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

bf17773

…rds compat Signed-off-by: Andrew Mason <andrew@planetscale.com>

migrate Status field out of wrapping structure

dbddd94

Signed-off-by: Andrew Mason <andrew@planetscale.com>

go mod tidy and small fix

85de1ce

Signed-off-by: Andrew Mason <andrew@planetscale.com>

extend parsing to include protobuf RequestTime

1eeb4cf

Signed-off-by: Andrew Mason <andrew@planetscale.com>

ajm188 force-pushed the andrew/vtctldserver-onlineddl-protobuf branch from 65e028c to 3dbbd40 Compare August 2, 2023 19:36

Andrew Mason added 5 commits August 2, 2023 15:56

vscode y did you not tell me about dis

14807cc

Signed-off-by: Andrew Mason <andrew@planetscale.com>

fix decode hook tests

e423990

Signed-off-by: Andrew Mason <andrew@planetscale.com>

remove e2e suites accidentally re-introduced

7a0e9dd

Signed-off-by: Andrew Mason <andrew@planetscale.com>

remove RequestTime, add WasReadyToComplete

e22973c

Signed-off-by: Andrew Mason <andrew@planetscale.com>

finish up deprecation warnings

26726cf

Signed-off-by: Andrew Mason <andrew@planetscale.com>

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 GuptaManan100, deepthi, frouioui, harshit-gangal, mattlord, notfelineit, rohit-nayak-ps, shlomi-noach and systay 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

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

Conversation

ajm188 commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Checklist

Deployment Notes

Uh oh!

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

Uh oh!

github-actions bot commented Mar 4, 2023

Uh oh!

github-actions bot commented Apr 6, 2023

Uh oh!

github-actions bot commented May 10, 2023

Uh oh!

github-actions bot commented May 17, 2023

Uh oh!

shlomi-noach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shlomi-noach left a comment

Choose a reason for hiding this comment

Uh oh!

shlomi-noach commented Aug 6, 2023

Uh oh!

ajm188 commented Aug 7, 2023

Uh oh!

ajm188 commented Aug 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ajm188 commented Jan 31, 2023 •

edited

Loading