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

mysql #2395

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

mysql #2395

wants to merge 1 commit into from

Conversation

serprex
Copy link
Contributor

@serprex serprex commented Dec 25, 2024

No description provided.

@serprex serprex force-pushed the mysql branch 13 times, most recently from 4fea228 to 319ebd3 Compare January 21, 2025 14:20
This was referenced Jan 21, 2025
serprex added a commit that referenced this pull request Jan 21, 2025
split out from #2395
@serprex serprex marked this pull request as ready for review January 21, 2025 16:15
serprex added a commit that referenced this pull request Jan 21, 2025
split out from #2395, where this allows tests to run against both PG & MySQL
@serprex serprex force-pushed the mysql branch 2 times, most recently from f40244b to 54d141b Compare January 21, 2025 16:35
qkind = qvalue.QValueKindInt64
case mysql.MYSQL_TYPE_SET:
qkind = qvalue.QValueKindInt64
case mysql.MYSQL_TYPE_TINY_BLOB:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unless you want different types down the line, can coalesce a lot of the case statements together

- 3306:3306
env:
MYSQL_ROOT_PASSWORD: cipass
#mariadb:
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO?

Copy link
Contributor Author

@serprex serprex Jan 22, 2025

Choose a reason for hiding this comment

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

TODO. I could not get CI to play nice with both services listening on 3306 even when trying to change port mapping

@@ -0,0 +1,3 @@
ALTER TABLE metadata_last_sync_state ADD COLUMN IF NOT EXISTS last_text text NOT NULL DEFAULT '';
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if you can store Pg and My states in a single column and run a migration in this file, shouldn't be too many rows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. PG has GREATEST logic around the integer value, whereas mysql does not

@@ -142,6 +142,7 @@ message MySqlConfig {
repeated string setup = 6;
uint32 compression = 7;
bool disable_tls = 8;
string flavor = 9;
Copy link
Contributor

Choose a reason for hiding this comment

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

could be an enum at the proto level?

return nil, nil
}

func (c *MySqlConnector) AddTablesToPublication(ctx context.Context, req *protos.AddTablesToPublicationInput) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there an analogue to this operation in MySQL at all? if not should be moved out of interface
same for below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's not, consumer is expected to filter. Moving stuff out of interface can be done in followup, there's plenty of places where we go with no-op implementations so that cleanup could be done more generally

}

func (c *MySqlConnector) GetMasterPos(ctx context.Context) (mysql.Position, error) {
showBinlogStatus := "SHOW BINARY LOG STATUS"
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is flavour dependent as well: https://mariadb.com/kb/en/show-binlog-status/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to have version checks be per-flavor too


rr, err := c.Execute(ctx, showBinlogStatus)
if err != nil {
return mysql.Position{}, fmt.Errorf("failed to SHOW BINARY LOG STATUS: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

inject actual command run here

case nil:
return qvalue.QValueNull(qkind), nil
case uint64:
// TODO unsigned integers
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO or in a followup?

Copy link
Contributor Author

@serprex serprex Jan 21, 2025

Choose a reason for hiding this comment

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

followup, most TODOs here at this point are looking to get resolved after merge

flow/connectors/mysql/mysql.go Outdated Show resolved Hide resolved
@@ -0,0 +1,117 @@
package connmysql
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this is needed

Copy link
Contributor Author

@serprex serprex Jan 21, 2025

Choose a reason for hiding this comment

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

binsyncer in go-mysql uses https://github.com/siddontang/go-log will add comment linking to this

serprex added a commit that referenced this pull request Jan 22, 2025
for pg we currently use destination instead of catalog. mysql->pg will use catalog
therefore metadata tables on destination pg can ignore this change,
avoiding issues upgrading existing pgpg setups

also update connector interfaces a bit

split out from #2395
@serprex serprex force-pushed the mysql branch 2 times, most recently from e67454b to 83d88a4 Compare January 22, 2025 15:12
serprex added a commit that referenced this pull request Jan 22, 2025
split from #2395 this will reduce the only changed lines to specialized lastOffset in flowable_core.go
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.

2 participants