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

Any plans on adding support for r2dbc spec #477

Closed
meshpaul opened this issue Jul 21, 2020 · 20 comments · Fixed by #736 or #914
Closed

Any plans on adding support for r2dbc spec #477

meshpaul opened this issue Jul 21, 2020 · 20 comments · Fixed by #736 or #914

Comments

@meshpaul
Copy link

Hello,

Any plans on adding support for r2dbc spec I think this is next evolution of DB connectivity

I think your driver already exposes some of these concepts.

Thank you.

@joschi
Copy link
Contributor

joschi commented Feb 23, 2021

@meshpaul Do you think the MySQL interface together with https://github.com/mirromutth/r2dbc-mysql could be used for this?

@smagellan
Copy link
Contributor

term "next evolution" may be problematic: https://twitter.com/jodastephen/status/1176186251277471747

@joschi
Copy link
Contributor

joschi commented Mar 23, 2021

term "next evolution" may be problematic: twitter.com/jodastephen/status/1176186251277471747

@smagellan That tweet was about the Asynchronous Database Access API (ADBA) project by Oracle, not R2DBC.

Also note that Oracle just released the official (!) R2DBC (!!) Driver for Oracle Database and even made it available on Maven Central (!!!), so I wouldn't exactly assume that R2DBC isn't a good candidate for implementation in a database driver. 😉

@smagellan
Copy link
Contributor

@joschi Interesting, thank you

@zhicwu
Copy link
Contributor

zhicwu commented Mar 24, 2021

Thanks everyone for sharing your thoughts on this. However, I'd not suggest to simply build an adapter to bridge async calls to the JDBC driver.

There's a diagram in #570 illustrates the whole structure. Basically, in order to implement r2dbc driver, we need to:

  • implement clickhouse-client and clickhouse-http-client/clickhouse-grpc-client/clickhouse-native-client first
    Note: clickhouse-jdbc will be refactored to use clickhouse-client.
  • and then work on clickhouse-r2dbc by leveraging clickhouse-client(and optionally http/grpc/native providers)

I'm working on clickhouse-client and clickhouse-http-client, which will be available in 0.3.1. clickhouse-gprc-client can be generated quickly, so it will be added soon. I believe we can start to work on clickhouse-r2dbc in 0.4.0 or 0.3.2. In case you need a quick workaround, please consider mysql/postgresql interface as @joschi suggested.

@sneakythr0ws
Copy link

sneakythr0ws commented May 11, 2021

Has anyone tried mysql interface for https://github.com/mirromutth/r2dbc-mysql?
Seems like this bundle doesn't work together.
I tried and had a problem

Caused by: java.lang.IllegalArgumentException: Cannot decode value of type class java.lang.String for 3 with collation 63
	at dev.miku.r2dbc.mysql.codec.DefaultCodecs.decodeNormal(DefaultCodecs.java:212) ~[r2dbc-mysql-0.8.2.RELEASE.jar:0.8.2.RELEASE]
	at dev.miku.r2dbc.mysql.codec.DefaultCodecs.decode(DefaultCodecs.java:105) ~[r2dbc-mysql-0.8.2.RELEASE.jar:0.8.2.RELEASE]
	at dev.miku.r2dbc.mysql.MySqlRow.get(MySqlRow.java:59) ~[r2dbc-mysql-0.8.2.RELEASE.jar:0.8.2.RELEASE]
	at dev.miku.r2dbc.mysql.MySqlConnection.lambda$null$3(MySqlConnection.java:86) ~[r2dbc-mysql-0.8.2.RELEASE.jar:0.8.2.RELEASE]
	at dev.miku.r2dbc.mysql.MySqlResult.processRow(MySqlResult.java:176) ~[r2dbc-mysql-0.8.2.RELEASE.jar:0.8.2.RELEASE]
	at dev.miku.r2dbc.mysql.MySqlResult.handleResult(MySqlResult.java:149) ~[r2dbc-mysql-0.8.2.RELEASE.jar:0.8.2.RELEASE]
	at dev.miku.r2dbc.mysql.MySqlResult.lambda$map$1(MySqlResult.java:93) ~[r2dbc-mysql-0.8.2.RELEASE.jar:0.8.2.RELEASE]
	at reactor.core.publisher.FluxHandle$HandleSubscriber.onNext(FluxHandle.java:102) ~[reactor-core-3.4.5.jar:3.4.5]
	... 51 common frames omitted

As I understand this is the same issue
@joschi have you try it?

@zhicwu zhicwu linked a pull request Oct 7, 2021 that will close this issue
@zhicwu
Copy link
Contributor

zhicwu commented Dec 29, 2021

Now we have clickhouse-client, a semi-async library supporting both http and grpc. Would be great if anyone is interested to start a PR or new repo for clickhouse-r2dbc :)

Update: look at benchmark results for implementation based on JDK 11 HttpClient, no doubt r2dbc can bring us better performance.

@zhicwu zhicwu linked a pull request May 2, 2022 that will close this issue
@zhicwu zhicwu added this to the 0.3.3 release milestone Sep 9, 2022
@linghengqian
Copy link

Just confused, doesn't #914 solve this issue? Why is this issue not closed?

@zhicwu
Copy link
Contributor

zhicwu commented Sep 11, 2022

Hi @linghengqian, it's because I haven't test the r2dbc driver yet. I just quickly went over the code, added a few feedback along with some quick fixes.

Among all the issues I mentioned here, my biggest concerns are 1) lack of testing; 2) potential resource leaking(closing ClickHouseResponse etc.), and 3) exception handling. Actually I'm not quite sure about the last two, as I'm not familiar with r2dbc spec. Having said that, I think @rernas35 did great job creating the new module and I wouldn't mind to release what we have as part of v0.3.3 :)

Now we have nightly build at here, can someone please give it a shot and comment here? Any pull request is greatly appreciated :)

@kyleloomis
Copy link

I tested the nightly build and I am running into two issues: Dialect and Package Version.

Issue with Dialect

The database configuration requires an R2dbcDialect. Error:

Factory method 'r2dbcCustomConversions' threw exception; nested exception is org.springframework.data.r2dbc.dialect.DialectResolver$NoDialectException: Cannot determine a dialect for ClickHouse using com.clickhouse.r2dbc.connection.ClickHouseConnectionFactory@7c70aae1. Please provide a Dialect.

I am configuring the client using AbstractR2dbcConfiguration

@Configuration
@EnableR2dbcRepositories(
    basePackages = ["com.base.repository.clickhouse"],
    databaseClientRef = "data"
)
class ClickhouseDBConfig(private val databaseCredentials: DatabaseCredentials) : AbstractR2dbcConfiguration() {
    override fun connectionFactory(): ConnectionFactory {
        return ConnectionFactories.get(databaseCredentials.CLICKHOUSE)
    }

    @Bean(name = ["data"])
    fun client(): DatabaseClient {
        val factory = connectionFactory()

        return DatabaseClient
            .builder()
            .bindMarkers(getDialect(factory).bindMarkersFactory)
            .connectionFactory(factory)
            .build()
    }
}

I am able to solve this issue by overriding the getDialect method available inside of AbstractR2dbcConfiguration.

override fun getDialect(connectionFactory: ConnectionFactory): R2dbcDialect {
    return MySqlDialect() # TODO replace with custom dialect
}

I am currently returning MySqlDialect() above (just for testing) and it seems be working. I believe we need to create a base dialect available inside of the R2DBC package.

Version Conflicts

There seems to be a conflict between the r2dbc-spi version when using both the clickhouse-r2dbc and org.springframework:spring-r2dbc:5.3.21 packages. When importing org.springframework:spring-r2dbc:5.3.21, it sets the r2dbc-spi version to 0.9.1.RELEASE. In this version, the RowMetadata interface has a deprecated getColumnNames method. If we use this default version of r2dbc-spi, then it fails because ClickHouseRowMetadata doesn't implement getColumnNames.

Alternatively, if I force the r2dbc-spi to be the version specified in clickhouse-r2dbc (1.0.0.RELEASE), then there is an error in the r2dbc core ColumnMapRowMapper class because getColumnNames no longer exists.

Since I imagine a lot of users will be using the package org.springframework:spring-r2dbc:5.3.21, I would recommend downgrading the r2dbc-spi version to be 0.9.1.RELEASE and implementing the getColumnNames inside of ClickHouseRowMetadata, at least until the spring-r2dbc package catches up to use the newest spi version.

@linghengqian
Copy link

linghengqian commented Sep 22, 2022

@kyleloomis Why do we need to fall back to R2DBC SPI 0.9.1.RELEASE? This is unreasonable, because at spring-projects/spring-framework#28787, spring-r2dbc has been promoted to R2DBC 1.0.0-RELEASE and no one in the R2DBC community is maintaining R2DBC driver for R2DBC SPI 0.9.1.RELEASE.

@mp911de
Copy link

mp911de commented Sep 22, 2022

R2DBC 0.9.x was an intermediate release with the goal of providing a transition phase towards R2DBC 1.0.

@kyleloomis
Copy link

Then perhaps we use 0.8.0 until there is broader adoption.

@zhicwu
Copy link
Contributor

zhicwu commented Sep 23, 2022

Thanks for the feedback @kyleloomis. Dialect is defined in Spring so it's not suitable to add it into r2dbc driver. As to version conflict, since r2dbc 0.9.1 is very close to 1.0.0, I created #1094 to support that. 0.8.x is probably too old and not worthy to support.

<dependency>
    <groupId>com.clickhouse</groupId>
    <artifactId>clickhouse-r2dbc_0.9.1</artifactId>
    <version>0.3.3-SNAPSHOT</version>
</dependency>

@meshpaul
Copy link
Author

Thanks everyone for sharing your thoughts on this. However, I'd not suggest to simply build an adapter to bridge async calls to the JDBC driver.

There's a diagram in #570 illustrates the whole structure. Basically, in order to implement r2dbc driver, we need to:

* implement `clickhouse-client` and `clickhouse-http-client`/`clickhouse-grpc-client`/`clickhouse-native-client` first
  Note: `clickhouse-jdbc` will be refactored to use `clickhouse-client`.

* and then work on `clickhouse-r2dbc` by leveraging `clickhouse-client`(and optionally http/grpc/native providers)

I'm working on clickhouse-client and clickhouse-http-client, which will be available in 0.3.1. clickhouse-gprc-client can be generated quickly, so it will be added soon. I believe we can start to work on clickhouse-r2dbc in 0.4.0 or 0.3.2. In case you need a quick workaround, please consider mysql/postgresql interface as @joschi suggested.

The original driver supports HTTP. In todays cloud protocols should CH R2DBC should support as bare minimum HTTP protocol IMHO

@zhicwu
Copy link
Contributor

zhicwu commented Nov 30, 2022

The original driver supports HTTP. In todays cloud protocols should CH R2DBC should support as bare minimum HTTP protocol IMHO

Sorry I missed this one. @meshpaul, have you tried the nightly build? You can use -http jar like this, which only contains http support.

@meshpaul
Copy link
Author

Thanks we will try

@meshpaul
Copy link
Author

And I also like gRPC as well approach will let you know

@zhicwu
Copy link
Contributor

zhicwu commented Jan 19, 2023

Like JDBC driver, there are 3 classifiers you can choose: all(CLI + gRPC + HTTP), grpc, and http.

@zhicwu zhicwu closed this as completed Jan 19, 2023
@hero-zhanghao
Copy link

Has anyone tried mysql interface for https://github.com/mirromutth/r2dbc-mysql? Seems like this bundle doesn't work together. I tried and had a problem

Caused by: java.lang.IllegalArgumentException: Cannot decode value of type class java.lang.String for 3 with collation 63
	at dev.miku.r2dbc.mysql.codec.DefaultCodecs.decodeNormal(DefaultCodecs.java:212) ~[r2dbc-mysql-0.8.2.RELEASE.jar:0.8.2.RELEASE]
	at dev.miku.r2dbc.mysql.codec.DefaultCodecs.decode(DefaultCodecs.java:105) ~[r2dbc-mysql-0.8.2.RELEASE.jar:0.8.2.RELEASE]
	at dev.miku.r2dbc.mysql.MySqlRow.get(MySqlRow.java:59) ~[r2dbc-mysql-0.8.2.RELEASE.jar:0.8.2.RELEASE]
	at dev.miku.r2dbc.mysql.MySqlConnection.lambda$null$3(MySqlConnection.java:86) ~[r2dbc-mysql-0.8.2.RELEASE.jar:0.8.2.RELEASE]
	at dev.miku.r2dbc.mysql.MySqlResult.processRow(MySqlResult.java:176) ~[r2dbc-mysql-0.8.2.RELEASE.jar:0.8.2.RELEASE]
	at dev.miku.r2dbc.mysql.MySqlResult.handleResult(MySqlResult.java:149) ~[r2dbc-mysql-0.8.2.RELEASE.jar:0.8.2.RELEASE]
	at dev.miku.r2dbc.mysql.MySqlResult.lambda$map$1(MySqlResult.java:93) ~[r2dbc-mysql-0.8.2.RELEASE.jar:0.8.2.RELEASE]
	at reactor.core.publisher.FluxHandle$HandleSubscriber.onNext(FluxHandle.java:102) ~[reactor-core-3.4.5.jar:3.4.5]
	... 51 common frames omitted

As I understand this is the same issue @joschi have you try it?

hello,I also met this problem, may I ask if there is a solution, or any other solution, thank you for any advice!

@sneakythr0ws @zhicwu

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

Successfully merging a pull request may close this issue.

9 participants