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.

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

Online DDL: ddl_strategy=direct #7172

Merged
merged 8 commits into from Dec 22, 2020

Conversation

shlomi-noach
Copy link
Contributor

@shlomi-noach shlomi-noach commented Dec 14, 2020

Backport

NO

Status

ready

Description

Following an internal discussion, where @askdba noticed this output:

 vtctlclient ApplySchema -sql="$(cat create_commerce_schema.sql)" commerce
Handling connection for 15999
I1213 17:07:05.817487   35734 main.go:64] I1213 14:07:05.661886 tablet_executor.go:259] Received DDL request. strategy =
I1213 17:07:05.882329   35734 main.go:64] I1213 14:07:05.729416 tablet_executor.go:259] Received DDL request. strategy =

strategy = (empty) seems confusing. Up till now, the empty string stood for the "direct" (non-online) DDL strategy.

Starting this PR, the dircet (non-online) strategy is named "direct". Consider the following examples:

mysql> select @@ddl_strategy;
+----------------+
| @@ddl_strategy |
+----------------+
| direct         |
+----------------+
1 row in set (0.00 sec)

mysql> alter table corder modify order_id int;
Query OK, 0 rows affected (0.03 sec)

mysql> set @@ddl_strategy='gh-ost';
Query OK, 0 rows affected (0.00 sec)

mysql> alter table corder modify order_id int;
+--------------------------------------+
| uuid                                 |
+--------------------------------------+
| 1305a8e4_3dd5_11eb_85ea_f875a4d24e90 |
+--------------------------------------+
1 row in set (0.00 sec)

mysql> set @@ddl_strategy='';
Query OK, 0 rows affected (0.00 sec)

mysql> alter table corder modify order_id int;
Query OK, 0 rows affected (0.01 sec)
/tmp/local$ vtctlclient ApplySchema -sql "alter table corder modify order_id int" commerce
I1214 08:25:23.239231 1630821 main.go:64] I1214 06:25:23.238050 tablet_executor.go:272] Received DDL request. strategy=direct
/tmp/local$ vtctlclient ApplySchema -ddl_strategy "gh-ost" -sql "alter table corder modify order_id int" commerce
I1214 08:25:38.917898 1630847 main.go:64] I1214 06:25:38.917187 tablet_executor.go:272] Received DDL request. strategy=gh-ost
I1214 08:25:38.918173 1630847 main.go:64] I1214 06:25:38.917839 tablet_executor.go:308] UUID=2ee751f1_3dd5_11eb_a6e5_f875a4d24e90
2ee751f1_3dd5_11eb_a6e5_f875a4d24e90
/tmp/local$ vtctlclient ApplySchema -ddl_strategy "" -sql "alter table corder modify order_id int" commerce
I1214 08:25:44.846210 1630860 main.go:64] I1214 06:25:44.844765 tablet_executor.go:272] Received DDL request. strategy=direct

notes:

  • in vtgate, the default value for -ddl_strategy command line flag is direct. An empty value is also interpreted as direct.
  • likewise, in vtctl/vtctlclient, the default value for -ddl_strategy command line flag is direct. An empty value is also interpreted as direct.

Related Issue(s)

Tracking: #6926

Todos

  • Tests
  • Documentation

Impacted Areas in Vitess

List general components of the application that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach
Copy link
Contributor Author

Documentation in vitessio/website#639

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@askdba
Copy link
Contributor

askdba commented Dec 14, 2020

Did we intentionally used dd_strategy instead of ddl_strategy?

likewise, in vtctl/vtctlclient, the default value for -dd_strategy command line flag is direct. An empty value is also interpreted as direct.```

@shlomi-noach
Copy link
Contributor Author

Did we intentionally used dd_strategy instead of ddl_strategy?

That's just a typo in my comment. Fixed.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@askdba
Copy link
Contributor

askdba commented Dec 17, 2020

On the MySQL CLI I've got the same results as above test case.

Query OK, 0 rows affected (0.00 sec)

MySQL [commerce]> alter table corder modify order_id int;
+--------------------------------------+
| uuid                                 |
+--------------------------------------+
| fc1d1705_404a_11eb_b95d_0242ac110002 |
+--------------------------------------+
1 row in set (0.00 sec)

MySQL [commerce]>
MySQL [commerce]>
MySQL [commerce]> set @@ddl_strategy='direct';
Query OK, 0 rows affected (0.00 sec)

MySQL [commerce]> alter table corder modify order_id int;
Query OK, 0 rows affected (0.01 sec)

But on the command line, I did not get the same output.

vitess@5c644a6f4170:/vt/local$ vtctlclient ApplySchema -sql "alter table corder modify order_id bigint" commerce
vitess@5c644a6f4170:/vt/local$ vtctl --version
ERROR: logging before flag.Parse: E1217 09:36:03.895258    1618 syslogger.go:149] can't connect to syslog
Version: d65877152 (Git branch 'ddl-strategy-direct') built on Thu Dec 17 09:15:19 UTC 2020 by vitess@ecd12ae46f97 using go1.13.15 linux/amd64

@shlomi-noach
Copy link
Contributor Author

But on the command line, I did not get the same output.

That's because since this PR was opened, I already merged another PR that removes that log line altogether. If you'd like the log line to appear please let me know.

@askdba
Copy link
Contributor

askdba commented Dec 17, 2020

I've found the log message useful. I'd like it back.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach
Copy link
Contributor Author

Log message is now restored.

@shlomi-noach
Copy link
Contributor Author

🙏 request for additional review

Copy link
Contributor

@ajm188 ajm188 left a comment

Choose a reason for hiding this comment

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

lgtm

… first ticker

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
…as 'direct' for strategy with options

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach shlomi-noach merged commit d4dedf4 into vitessio:master Dec 22, 2020
@shlomi-noach shlomi-noach deleted the ddl-strategy-direct branch December 22, 2020 10:48
@askdba askdba added this to the v9.0 milestone Dec 23, 2020
// DDLStrategyGhost requests gh-ost to run the migration
DDLStrategyGhost DDLStrategy = "gh-ost"
// DDLStrategyPTOSC requests pt-online-schema-change to run the migration
DDLStrategyPTOSC DDLStrategy = "pt-osc"
)

// IsDirect returns true if this strategy is a direct strategy
// A strategy is direct if it's not explciitly one of the online DDL strategies
Copy link
Contributor

Choose a reason for hiding this comment

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

nit pick: explciitly

@@ -232,6 +232,7 @@ func (exec *TabletExecutor) executeSQL(ctx context.Context, sql string, execResu
return nil
}
}
exec.wr.Logger().Infof("Received DDL request. strategy=%+v", schema.DDLStrategyDirect)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it is useful to log the sql too 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants