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

DBZ-7085 Add MariaDB driver and use with MariaDB profile #4975

Merged
merged 7 commits into from
Nov 9, 2023

Conversation

Naros
Copy link
Member

@Naros Naros commented Nov 2, 2023

@Naros Naros added the 2.5 label Nov 2, 2023
@Naros Naros changed the title DBZ-7085 Fix test suite to protocol-based factory URL DBZ-7085 Add MariaDB driver and use with MariaDB profile Nov 2, 2023
.with(MySqlConnectorConfig.JDBC_PROTOCOL, System.getProperty("database.protocol",
MySqlConnectorConfig.JDBC_PROTOCOL.defaultValueAsString()))
.with(MySqlConnectorConfig.JDBC_DRIVER, System.getProperty("database.jdbc.driver",
MySqlConnectorConfig.JDBC_DRIVER.defaultValueAsString()))
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make it easier for users when they have a single switch that sets the backend to on of the supported MySQL types, like mysql, percona, mariadb, etc and we set related parameters accordingly instead of providing that full detail level?

Copy link
Member Author

@Naros Naros Nov 3, 2023

Choose a reason for hiding this comment

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

Yes, there is a Jira to introduce a strategy pattern for these use cases. But rather than shoehorn that into the code, we're doing this piecemeal so we see all the touch points we need to refactor more clearly over the 2.5 release cycle.

Configuration options like this definitely need to be dealt with but we also have to be a bit wary of how we change this so that its both backward compatible within the 2.x series and that we don't break some of the recent changes introduced to support drivers such as the AWS-specific driver so users can leverage the IAM authentication offered by AWS.

@@ -57,6 +57,10 @@
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.mariadb.jdbc</groupId>
<artifactId>mariadb-java-client</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about this but maybe should drivers be provided scope?!

Copy link
Member Author

Choose a reason for hiding this comment

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

We've always shipped all open-source drivers with the connectors to simplify the installation for users. If a user needs or requires a specific driver, they're free to replace the driver as needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, both drivers hould be shipped, esp. if they can co-exist side-by-side.

Per https://jira.mariadb.org/browse/CONJ-977, the MariaDB team does not
believe that the Integer.MIN_VALUE solution for triggering streaming is
spec compliant and as of MariaDB 3.x+ drivers, they explicitly will not
allow this.

This change explicitly sets the fetchSize to 1 to mimic the same style
of behavior that the default fetchSize uses when using MySQL. If a user
provides a custom fetchSize, that will take precedences.
@Naros
Copy link
Member Author

Naros commented Nov 3, 2023

Speaking of co-existence @jpechane, see my recent commit about "fetchSize".

Per https://jira.mariadb.org/browse/CONJ-977, the MariaDB team does not
believe that the Integer.MIN_VALUE solution for triggering streaming is
spec compliant and as of MariaDB 3.x+ drivers, they explicitly will not
allow this.

This change explicitly sets the fetchSize to 1 to mimic the same style
of behavior that the default fetchSize uses when using MySQL. If a user
provides a custom fetchSize, that will take precedence.

I'm going to start recording these nuances in that strategy Jira so we don't loose track.

@dwdraju
Copy link

dwdraju commented Nov 6, 2023

FYI, the link to download Mariadb connector plug-in is broken in v2.5 release page.

@Naros
Copy link
Member Author

Naros commented Nov 6, 2023

FYI, the link to download Mariadb connector plug-in is broken in v2.5 release page.

We'll have to adjust the logic in the UI bits to avoid adding that entry as really technically the MariaDB link should be the MySQL connector but because of how we listed the databases in the test matrix, that happened.

Added debezium/debezium.github.io#969 to test a way to fix this.

@Naros
Copy link
Member Author

Naros commented Nov 7, 2023

@jpechane so assuming that the tests pass now (I confirmed that the failures in the previous run at least passed locally now with my recent changes), this should be ready for a review. I added a note to the strategy Jira DBZ-7083 about the protocol changes in the connection configuration class so we can find a better solution in the later refactoring we intend to do.

Through my discussions with the MariaDB guys, it seems that they suggested using #getString() for the temporal types in the handler class since MariaDB does not allow the use of #getBlob() like MySQL does.

@Naros
Copy link
Member Author

Naros commented Nov 8, 2023

So it looks like MySQL's handling of TIMETSTAMP default values differs from MariaDB. I'm looking into this, but I'm unsure whether there is a viable way to handle this uniformly across both since removing timezone=auto fixes this particular test failure but causes other tests to fail on the actual column values themselves due to timezone handling behavior.

I'm almost inclined to say that the MySQL behavior may be what is wrong in these two tests, particularly with the fact that the outcome of the MariaDB test is this:

org.junit.ComparisonFailure: 
Expected :"1970-01-01T00:00:01-05:00"
Actual   :"1970-01-01T06:00:01-05:00"

That 6 hour difference makes sense given the server is US/Somoa and I'm US/New_York, which is a 6 hours difference. wdyt?

@Naros
Copy link
Member Author

Naros commented Nov 8, 2023

After an in-depth discussion with some folks with MariaDB, we've concluded that both drivers are working correctly but reacting to the default value information differently due to how the two drivers manage time zones.

For MySQL, the connectionTimeZone actually tells the driver to do value manipulation at the driver level, which means that when the connector executes SHOW CREATE TABLE, the data returned, in this case, is simply a string. The time function on the server is unaware of the connectionTimeZone setting. This is why MySQL, if a timestamp has a default value of 1970-01-01 00:00:01, gets returned unaffected.

MariaDB takes a completely different approach. In their case, their timezone=auto setting changes the physical timezone in the session at the server level, meaning that not only does the server return the values to us with using the same timezone like MySQL's connectionTimeZone when set to SERVER, but it also affects any rendering of timestamps via server-side calls, like the one that generates the output for SHOW CREATE TABLE.

So it seems logical that we need to adjust the MySqlDefaultValueIT test for MariaDB to account for this, but I'd like your thoughts @jpechane if you maybe have some idea I haven't explored.

@jpechane
Copy link
Contributor

jpechane commented Nov 8, 2023

@Naros I don't think there is other solution. Let's just modify the test to account for it.

@Naros Naros requested a review from jpechane November 8, 2023 18:16
@Naros
Copy link
Member Author

Naros commented Nov 8, 2023

Test failures are unrelated.

@jpechane jpechane merged commit da9c2bf into debezium:main Nov 9, 2023
26 of 30 checks passed
@jpechane
Copy link
Contributor

jpechane commented Nov 9, 2023

@Naros Aplied, thanks a lot

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