-
Notifications
You must be signed in to change notification settings - Fork 591
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
fix(meta): fix & enable e2e tests with MySQL meta backend #19846
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -0,0 +1,4 @@ | |||
disallowed-methods = [ | |||
{ path = "sea_query::table::column::ColumnDef::binary", reason = "Please use `.rw_binary(..)` instead." }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain it by some comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's explained in docs:
/// Set column type as `longblob` for MySQL, `bytea` for Postgres, and `blob` for Sqlite.
///
/// Should be preferred over [`binary`](ColumnDef::binary) or [`blob`](ColumnDef::blob) for large binary fields,
/// typically the fields wrapping protobuf or other serialized data. Otherwise, MySQL will return an error
/// when the length exceeds 65535 bytes.
Signed-off-by: Bugen Zhao <[email protected]>
.add_column(ColumnDef::new(Connection::Params).binary().not_null()) | ||
.add_column( | ||
ColumnDef::new(Connection::Params) | ||
.rw_binary(manager) | ||
.not_null(), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct to directly change an existing migration script? Not sure, just want to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only for MySQL, I think it's okay.
Signed-off-by: Bugen Zhao [email protected]I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Follow up of #19595. Address bugs for MySQL meta backend and re-enable it.
Checklist
Documentation
Release note