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

Adding support Mariadb events, such as GTID and annotate rows event(sql) #45

Merged
merged 3 commits into from
Aug 27, 2022

Conversation

wingerx
Copy link

@wingerx wingerx commented Jul 13, 2021

  1. Change EventType to use eventNumber not ordinal().
    ANNOTATE_ROWS(160), MARIADB_GTID(162) MARIADB_GTID_LIST(163)
  2. Add MariaDB event data implements, not only types.
  3. Now MariaDB can support query(sql) event and GTID events

@osheroff pls check this PR, ths.

@osheroff
Copy link
Owner

generally speaking I'd prefer inheritance over a bunch of if statements. Can we try to make MariaDBBinaryLogConnector class that inherits from the base class and changes functionality that way?

I'd also like to see quite a few more tests around mariaDB functionality, if possible -- especially ensuring that we test all the various deserializers.

@wingerx
Copy link
Author

wingerx commented Jul 15, 2021

generally speaking I'd prefer inheritance over a bunch of if statements. Can we try to make MariaDBBinaryLogConnector class that inherits from the base class and changes functionality that way?

I'd also like to see quite a few more tests around mariaDB functionality, if possible -- especially ensuring that we test all the various deserializers.

ok, i will try to refact.

import com.github.shyiko.mysql.binlog.event.deserialization.EventDeserializer.EventDataWrapper;
import com.github.shyiko.mysql.binlog.event.deserialization.GtidEventDataDeserializer;
import com.github.shyiko.mysql.binlog.event.deserialization.QueryEventDataDeserializer;
import com.github.shyiko.mysql.binlog.event.deserialization.RotateEventDataDeserializer;
Copy link
Owner

Choose a reason for hiding this comment

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

please leave the import style alone. everyone's editor has an opinion about how imports should be done and they're always changing back and forth. I personally do not care even a little bit but I'm tired of it changing all the time.

}
}

protected GtidSet newGtidSet(String gtidSet) {
Copy link
Owner

Choose a reason for hiding this comment

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

rename: buildGtidSet

gtidSet = new GtidSet(fetchGtidPurged());
}
}
setupGtidToPurgedIfNeeded();
Copy link
Owner

Choose a reason for hiding this comment

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

rename: setupGtidSet

@@ -592,8 +572,7 @@ public void connect() throws IOException, IllegalStateException {
ensureEventDataDeserializer(EventType.ROTATE, RotateEventDataDeserializer.class);
synchronized (gtidSetAccessLock) {
if (gtidSet != null) {
ensureEventDataDeserializer(EventType.GTID, GtidEventDataDeserializer.class);
ensureEventDataDeserializer(EventType.QUERY, QueryEventDataDeserializer.class);
ensureGtidEventDataDeserializerIfNeeded();
Copy link
Owner

Choose a reason for hiding this comment

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

rename: ensureGtidEventDataDeserializer

QueryEventData queryEventData = (QueryEventData) EventDataWrapper.internal(event.getData());
String sql = queryEventData.getSql();
case ANNOTATE_ROWS:
String sql = getQuerySql(event);
Copy link
Owner

Choose a reason for hiding this comment

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

if you look at the implementation of getQuerySql, you'll see that this is false sharing -- in the case of MariaDB's QUERY event it bounces right back to the mysql-compatible getQuerySql.

Therefore -- keep the implementation of the QUERY case the same, just make a different impl for maria.


@Override
protected void setupGtidToPurgedIfNeeded() throws IOException {
//Mariadb ignore
Copy link
Owner

Choose a reason for hiding this comment

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

does maria-db not support the gtid_purged variable?

Copy link
Author

Choose a reason for hiding this comment

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

@Override
protected void ensureGtidEventDataDeserializerIfNeeded() {
if (isUseSendAnnotateRowsEvent()) {
ensureEventDataDeserializer(EventType.ANNOTATE_ROWS, AnnotateRowsEventDataDeserializer.class);
Copy link
Owner

Choose a reason for hiding this comment

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

i don't think we need all the isUseSendAnnotate stuff. It's not going to hurt anything to have an unsed data deserializer in there.

@osheroff
Copy link
Owner

needs some documentation about setting maria-db specific flags and such

@gunnarmorling
Copy link
Collaborator

Hey @jpechane, @Naros, check this one out, it will probably be relevant for the MariaDB exploration within Debezium.

Great effort, @wingerx!

@wingerx
Copy link
Author

wingerx commented Jul 23, 2021

No, i think they are similar only in Mariadb

dumpBinaryLogCommand = new DumpBinaryLogCommand(serverId, "", 0L, isUseSendAnnotateRowsEvent());

} else {
dumpBinaryLogCommand = new DumpBinaryLogCommand(serverId, getBinlogFilename(), getBinlogPosition());

Choose a reason for hiding this comment

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

@scottmf
Copy link

scottmf commented Nov 14, 2021

hi, I haven't seen any activity on this in a while. Is this is a dead PR or is there a chance of it moving forward? I, like many others, am eagerly awaiting GTID support for mariadb :)

@arman1371
Copy link

Hi
Thanks for your effort.
Is there any activity here? Can we use this development branch and hope this will be merged soon?

@ivapiv
Copy link

ivapiv commented Aug 10, 2022

Hi @osheroff, could you maybe share any updates regarding the status in this PR? If you could let us know what is missing in order the changes to make it on master, would be happy to contribute and help to move forward with it.

@osheroff
Copy link
Owner

trying to get this going here #79

@osheroff osheroff merged commit 8123277 into osheroff:master Aug 27, 2022
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.

8 participants