Skip to content

Conversation

@Torch3333
Copy link
Contributor

@Torch3333 Torch3333 commented Sep 4, 2025

Description

This adds support for TiDB versions 6.5, 7.5, and 8.5, a fully MySQL-compliant database. Connection to TiDB is done through the MySQL Connector/J JDBC driver.

A sample ScalarDB configuration to use TiDB is as follows.

scalar.db.storage=jdbc
scalar.db.contact_points=jdbc:mysql://localhost:4000/
scalar.db.username=root
scalar.db.password=tidb

Related issues and/or PRs

N/A

Changes made

  • Adds TiDB integration tests to the CI
  • Adds RdbEngine for TiDB

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes.
  • I have considered whether similar issues could occur in other products, components, or modules if this PR is for bug fixes.
  • Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
  • Tests (unit, integration, etc.) have been added for the changes.
  • My changes generate no new warnings.
  • Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

Release notes

TiDB versions 6.5, 7.5, and 8.5 are now supported as underlying storage by using MySQL Connector/J JDBC driver.

@Torch3333 Torch3333 self-assigned this Sep 4, 2025
@Torch3333 Torch3333 added the enhancement New feature or request label Oct 3, 2025
@Torch3333 Torch3333 marked this pull request as ready for review October 3, 2025 08:05
@Torch3333 Torch3333 requested review from a team, brfrn169, Copilot, feeblefakie and komamitsu and removed request for a team October 3, 2025 08:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds TiDB support to ScalarDB as an underlying storage by enabling connection through MySQL Connector/J JDBC driver. It supports TiDB versions 6.5, 7.5, and 8.5, with configuration to override the default SERIALIZABLE isolation level to REPEATABLE_READ or READ_COMMITTED since TiDB doesn't support SERIALIZABLE.

  • Added isolation level configuration support in JDBC environment setup
  • Implemented TiDB-specific handling for unsupported data types in import tests
  • Added comprehensive CI integration tests for TiDB versions 6.5, 7.5, and 8.5

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
core/src/integration-test/java/com/scalar/db/storage/jdbc/JdbcEnv.java Adds isolation level configuration property support for test environment
core/src/integration-test/java/com/scalar/db/storage/jdbc/JdbcAdminImportTestUtils.java Implements TiDB detection and specific data type filtering for import tests
.github/workflows/ci.yaml Adds CI integration test jobs for TiDB versions 6.5, 7.5, and 8.5

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@brfrn169
Copy link
Collaborator

brfrn169 commented Oct 7, 2025

Since the ScalarDB transaction API sets a SERIALIZABLE isolation level by default not supported by TiDB

@Torch3333 I don’t think we set the SERIALIZABLE isolation level by default. We only set the isolation level when users specify the scalar.db.jdbc.isolation_level property. What do you think?

@feeblefakie
Copy link
Contributor

@Torch3333 What would happen if users don't specify scalardb.jdbc.isolation_level=REPEATABLE_READ or READ-COMMITTED?

BTW, TiDB seems to be linearizable, independent of isolation levels. So, any isolation levels can probaly be fine for correctness. (cc: @brfrn169 )
https://docs.pingcap.com/tidb/stable/system-variables/#tidb_guarantee_linearizability-new-in-v50

# Conflicts:
#	.github/workflows/ci.yaml
@Torch3333
Copy link
Contributor Author

Torch3333 commented Oct 20, 2025

@brfrn169 @feeblefakie @komamitsu

@Torch3333 I don’t think we set the SERIALIZABLE isolation level by default. We only set the isolation level when users specify the scalar.db.jdbc.isolation_level property. What do you think?

You are right. I mistakenly thought ScalarDB was setting the JDBC transaction isolation level for all the transaction managers, but in fact, it is only set for the JDBC transaction manager, not for Consensus Commit.
As discussed with @brfrn169 and @feeblefakie, we decided that setting it for the JDBC transaction manager was not required. From now on, the storage default isolation level will be used. For better clarity for our users, I made these changes in a separate PR #3076

What would happen if users don't specify scalardb.jdbc.isolation_level=REPEATABLE_READ or READ-COMMITTED?

TiDB uses the REPEATABLE-READ as the default level. READ-COMMITED is also an accepted value. Setting SERIALIZABLE will cause the driver to throw this error. The documentation explains it here.

The isolation level 'SERIALIZABLE' is not supported. Set tidb_skip_isolation_level_check=1 to skip this error

Other than that, I finally found a document listing the features of the MySQL Connector/J driver that TiDB does not support. As far as my understanding goes, I don't think ScalarDB uses any of the incompatibilities, so this confirms that using this driver instead of the TiDB modified version of Connector/J is fine.

@Torch3333
Copy link
Contributor Author

I will put this PR in draft until this blocking dependent PR is merged.
#3076

@Torch3333 Torch3333 marked this pull request as draft October 20, 2025 06:23
@Torch3333
Copy link
Contributor Author

@komamitsu You raised a point during the demo meeting more than 2 weeks ago about how the driver handles load balancing. There seems to be a library to handle just that, but I will investigate it once other higher-priority tasks are completed.

@feeblefakie
Copy link
Contributor

@Torch3333 This can be opened again now?

Copy link
Contributor Author

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

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

The PR is ready for review again.
Please review everything from scratch once again, since there are numerous new files since your last review.
Please take a look at my explanation below about the changes.

@Torch3333 Torch3333 requested a review from komamitsu October 24, 2025 05:49
@Torch3333 Torch3333 marked this pull request as ready for review October 24, 2025 05:49
Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@Torch3333 Torch3333 requested a review from komamitsu October 28, 2025 01:28
Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@brfrn169 brfrn169 merged commit 367eb2e into master Oct 28, 2025
125 of 128 checks passed
@brfrn169 brfrn169 deleted the tidb_support branch October 28, 2025 16:15
Torch3333 added a commit that referenced this pull request Oct 29, 2025
# Conflicts:
#	core/src/integration-test/java/com/scalar/db/storage/jdbc/ConsensusCommitAdminIntegrationTestWithJdbcDatabase.java
#	core/src/integration-test/java/com/scalar/db/storage/jdbc/JdbcAdminCaseSensitivityIntegrationTest.java
#	core/src/integration-test/java/com/scalar/db/storage/jdbc/JdbcAdminIntegrationTest.java
#	core/src/integration-test/java/com/scalar/db/storage/jdbc/SingleCrudOperationTransactionAdminIntegrationTestWithJdbcDatabase.java
#	core/src/integration-test/java/com/scalar/db/transaction/jdbc/JdbcTransactionAdminIntegrationTest.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants