r2dbc module is added. by rernas35 · Pull Request #914 · ClickHouse/clickhouse-java

rernas35 · 2022-05-02T09:11:25Z

No description provided.

CLAassistant · 2022-05-02T09:11:31Z

All committers have signed the CLA.

github-actions · 2022-05-02T09:28:00Z

Benchmark                                (client)  (connection)  (statement)  (type)   Mode  Cnt     Score    Error  Units
Basic.insertOneRandomNumber  clickhouse-http-jdbc         reuse       normal  object  thrpt   20   261.588 ± 24.470  ops/s
Basic.insertOneRandomNumber  clickhouse-http-jdbc         reuse     prepared  object  thrpt   20   246.439 ± 30.610  ops/s
Basic.insertOneRandomNumber  clickhouse-http-jdbc           new       normal  object  thrpt   20   269.055 ± 25.420  ops/s
Basic.insertOneRandomNumber  clickhouse-http-jdbc           new     prepared  object  thrpt   20   249.802 ± 22.271  ops/s
Basic.insertOneRandomNumber  clickhouse-grpc-jdbc         reuse       normal  object  thrpt   20   271.438 ± 37.502  ops/s
Basic.insertOneRandomNumber  clickhouse-grpc-jdbc         reuse     prepared  object  thrpt   20   240.271 ± 20.264  ops/s
Basic.insertOneRandomNumber  clickhouse-grpc-jdbc           new       normal  object  thrpt   20   271.611 ± 38.994  ops/s
Basic.insertOneRandomNumber  clickhouse-grpc-jdbc           new     prepared  object  thrpt   20   228.747 ± 32.263  ops/s
Basic.selectOneRandomNumber  clickhouse-http-jdbc         reuse       normal  object  thrpt   20  1045.156 ± 75.673  ops/s
Basic.selectOneRandomNumber  clickhouse-http-jdbc         reuse     prepared  object  thrpt   20  1078.172 ± 86.075  ops/s
Basic.selectOneRandomNumber  clickhouse-http-jdbc           new       normal  object  thrpt   20  1112.164 ± 90.219  ops/s
Basic.selectOneRandomNumber  clickhouse-http-jdbc           new     prepared  object  thrpt   20  1133.879 ± 92.128  ops/s
Basic.selectOneRandomNumber  clickhouse-grpc-jdbc         reuse       normal  object  thrpt   20   929.353 ± 50.465  ops/s
Basic.selectOneRandomNumber  clickhouse-grpc-jdbc         reuse     prepared  object  thrpt   20   941.641 ± 42.785  ops/s
Basic.selectOneRandomNumber  clickhouse-grpc-jdbc           new       normal  object  thrpt   20   919.707 ± 77.469  ops/s
Basic.selectOneRandomNumber  clickhouse-grpc-jdbc           new     prepared  object  thrpt   20   921.545 ± 74.374  ops/s

zhicwu · 2022-05-02T09:56:09Z

Thank you @rernas35! Just some immediate feedback:

Is JDK 11 a hard requirement? Can we stay on JDK 8?
Least dependency - can we remove optional dependencies like lombok and slf4j? com.clickhouse.client.logging.Logger can be used for logging - it falls back to java util logging when slf4j is not available.
Can we merge clickhouse-r2dbc-runtime-test into clickhouse-r2dbc?
Bump R2DBC to 1.0.0.RELEASE? - see R2DBC 1.0 goes GA

There're some other minor issues in pom files but once you fixed build break and tidy up code, I can follow up to address them.

linghengqian · 2022-06-13T05:20:26Z

Is the failure of the CI/CD expected behavior or does it mean that the PR has not yet completed? Is there any unfinished business？

rernas35 · 2022-06-13T18:42:28Z

Is the failure of the CI/CD expected behavior or does it mean that the PR has not yet completed? Is there any unfinished business？

can you please approve the workflows

rernas35 · 2022-06-14T10:12:47Z

Is the failure of the CI/CD expected behavior or does it mean that the PR has not yet completed? Is there any unfinished business？

builds fixed

mzitnik · 2022-06-25T19:19:53Z

@rernas35 i see this pr is still pending
can you explain shorty what is the purpose of the new module

rernas35 · 2022-06-25T22:17:38Z

@rernas35 i see this pr is still pending can you explain shorty what is the purpose of the new module

this is r2dbc module which is compliant with r2dbc specs (https://r2dbc.io/). I need to add a sample and readme then it is ready for review

zhicwu · 2022-06-26T14:06:32Z

Hi @rernas35, it looks like the test never ran. The build success only proves that r2dbc module can be compiled without issue. I think it's better to enable tests and merge it into 0.3.3 release.

zhicwu

Can we also move clickhouse-r2dbc-samples to examples directory?

clickhouse-r2dbc/pom.xml

clickhouse-r2dbc/src/main/java/com/clickhouse/r2dbc/ClickHouseBatch.java

clickhouse-r2dbc/src/main/java/com/clickhouse/r2dbc/ClickHouseRow.java

clickhouse-r2dbc/src/main/java/com/clickhouse/r2dbc/types/ClickHouseType.java

clickhouse-r2dbc/pom.xml

zhicwu · 2022-06-27T00:48:21Z

@rernas35, please use ClickHouseNodes.of(uri) to get list of servers to connect to, since load balancing and failover are now supported(details at here). Please pay attention to failover, it's implemented by a wrapper around ClickHouseClient - see implementation at here. If you don't want the wrapper, you have to explicitly disable agent when build ClickHouseClient, for example: ClickHouseClient.builder().agent(false)...build().

rernas35 · 2022-07-17T11:14:12Z

@rernas35, please use ClickHouseNodes.of(uri) to get list of servers to connect to, since load balancing and failover are now supported(details at here). Please pay attention to failover, it's implemented by a wrapper around ClickHouseClient - see implementation at here. If you don't want the wrapper, you have to explicitly disable agent when build ClickHouseClient, for example: ClickHouseClient.builder().agent(false)...build().

@zhicwu I have done some tests but it looks like r2dbc url is not supporting multiple hosts currently.

here

Since currently fragment part of r2dbc url is not supported, tags are not supported but other parameters are all passed and ClickHouseNodes is being used.

rernas35 · 2022-07-17T11:17:59Z

@rernas35 i see this pr is still pending can you explain shorty what is the purpose of the new module

this is r2dbc module which is compliant with r2dbc specs (https://r2dbc.io/). I need to add a sample and readme then it is ready for review

resolved

rernas35 · 2022-07-17T11:19:03Z

@rernas35 i see this pr is still pending can you explain shorty what is the purpose of the new module

this module will enable client to use clickhouse-jdbc functionality in a r2dbc compliant way

zhicwu · 2022-09-07T03:16:37Z

Thank you @rernas35, really appreciate your efforts on this. Will review this in the coming weekend and merge it into develop if no critical issues.

zhicwu

A few more feedback(just for documentation):

needs a place to close ClickHouseResponse in case query was cancelled etc.
exception handling should be improved
exception when using grpc - try mvn -Dprotocol=grpc clean verify
test/resources can be safely removed as it's included in clickhouse-client test-jar
rename examples directory to r2dbc for consistency
may remove apache commons dependency

zhicwu · 2022-09-11T01:04:35Z

clickhouse-r2dbc/pom.xml

+        </dependency>
+        <dependency>
+            <groupId>com.clickhouse</groupId>
+            <artifactId>clickhouse-jdbc</artifactId>


Should be restricted to test scope

zhicwu · 2022-09-11T01:05:22Z

clickhouse-r2dbc/pom.xml

+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-api</artifactId>
+            <version>5.8.2</version>


Declare version as property

zhicwu · 2022-09-11T01:05:43Z

clickhouse-r2dbc/pom.xml

+        <plugins>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-surefire-plugin</artifactId>


Should be failsafe plugin as it's integration test

zhicwu · 2022-09-11T01:06:05Z

clickhouse-r2dbc/src/main/java/com/clickhouse/r2dbc/ClickHouseBatch.java

+
+    private static final ClickHouseFormat PREFERRED_FORMAT = ClickHouseFormat.TabSeparatedWithNamesAndTypes;
+    private ClickHouseRequest<?> request;
+    List<String> sqlList = new ArrayList<>();


Add private final?

zhicwu · 2022-09-11T01:10:33Z

clickhouse-r2dbc/src/main/java/com/clickhouse/r2dbc/connection/ClickHouseConnection.java

+        if (isHttp()) {
+            req = req.set("send_progress_in_http_headers", 1);
+        }


This is no longer required since ClickHouse 22.6 - see #947

zhicwu · 2022-09-11T01:10:54Z

...r2dbc/src/main/java/com/clickhouse/r2dbc/connection/ClickHouseConnectionFactoryProvider.java

+    private static final List<String> connQueryParams = Arrays.asList("auto_discovery", "node_discovery_interval",
+            "node_discovery_limit", "load_balancing_policy", "load_balancing_tags", "health_check_method",
+            "health_check_interval", "check_all_nodes", "node_check_interval", "node_group_size", "failover",
+            "retry");


Should consider all client options

zhicwu · 2022-09-11T01:11:11Z

...r2dbc/src/main/java/com/clickhouse/r2dbc/connection/ClickHouseConnectionFactoryProvider.java

+
+        StringBuilder urlBuilder = new StringBuilder(String.format("%s://%s/%s?user=%s&password=%s", protocol, hosts, database, username, password));
+        String params = connQueryParams.stream().filter(queryParam -> cfOpt.getValue(Option.valueOf(queryParam)) != null)
+                .map(queryParam -> String.format("%s=%s", queryParam, cfOpt.getValue(Option.valueOf(queryParam))))


zhicwu · 2022-09-11T01:11:30Z

...r2dbc/src/main/java/com/clickhouse/r2dbc/connection/ClickHouseConnectionFactoryProvider.java

+        StringBuilder urlBuilder = new StringBuilder(String.format("%s://%s/%s?user=%s&password=%s", protocol, hosts, database, username, password));
+        String params = connQueryParams.stream().filter(queryParam -> cfOpt.getValue(Option.valueOf(queryParam)) != null)
+                .map(queryParam -> String.format("%s=%s", queryParam, cfOpt.getValue(Option.valueOf(queryParam))))
+                .collect(Collectors.joining("%"));


Is it a typo? Should be '&'?

zhicwu · 2022-09-11T01:11:51Z

...khouse-r2dbc/src/main/java/com/clickhouse/r2dbc/connection/ClickHouseConnectionMetadata.java

+    final ClickHouseClient client;
+    final ClickHouseNode server;
+
+    private final String serverVersion = null;


Why it's always null?

zhicwu · 2022-09-11T01:12:32Z

clickhouse-r2dbc/src/test/java/com/clickhouse/r2dbc/spi/test/R2DBCTestKitImplTest.java

+    @BeforeAll
+    public static void setup() throws Exception {
+        ClickHouseServerForTest.beforeSuite();
+        connectionFactory = ConnectionFactories.get(


Should support more protocols instead of just http

zhicwu · 2022-09-11T01:16:14Z

@rernas35, no action required. Just document some of the feedback and I'll make changes on your branch shortly to address most of them and then merge. Stay tuned :)

zhicwu changed the base branch from master to develop May 2, 2022 09:39

zhicwu linked an issue May 2, 2022 that may be closed by this pull request

Any plans on adding support for r2dbc spec #477

Closed

rernas35 force-pushed the r2dbc_feature branch 7 times, most recently from d9a6f24 to 620195c Compare May 4, 2022 18:41

rernas35 force-pushed the r2dbc_feature branch from 3fead38 to 8abbf21 Compare June 13, 2022 18:31

rernas35 force-pushed the r2dbc_feature branch from b86144e to af0065c Compare June 25, 2022 22:12

rernas35 force-pushed the r2dbc_feature branch from af0065c to 281c39f Compare June 25, 2022 22:21

zhicwu added this to the 0.3.3 release milestone Jun 26, 2022

zhicwu suggested changes Jun 26, 2022

View reviewed changes

rernas35 force-pushed the r2dbc_feature branch 5 times, most recently from 6791f9b to 50bc36c Compare July 17, 2022 08:26

rernas35 force-pushed the r2dbc_feature branch 2 times, most recently from 5af2de5 to 14f931f Compare July 24, 2022 21:49

rernas35 force-pushed the r2dbc_feature branch 2 times, most recently from 4854205 to 49976f2 Compare September 4, 2022 08:43

rernas35 force-pushed the r2dbc_feature branch 4 times, most recently from 41e1afc to 4bb1bea Compare September 5, 2022 08:54

first version of r2dbc

5b26f1c

rernas35 force-pushed the r2dbc_feature branch from 4bb1bea to 5b26f1c Compare September 5, 2022 10:37

rernas35 requested a review from zhicwu September 5, 2022 12:38

zhicwu reviewed Sep 11, 2022

View reviewed changes

Fit and finish

dc23b90

zhicwu merged commit 19ab569 into ClickHouse:develop Sep 11, 2022

linghengqian mentioned this pull request Sep 11, 2022

Any plans on adding support for r2dbc spec #477

Closed

Conversation

rernas35 commented May 2, 2022

Uh oh!

CLAassistant commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 2, 2022

Uh oh!

zhicwu commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linghengqian commented Jun 13, 2022

Uh oh!

rernas35 commented Jun 13, 2022

Uh oh!

rernas35 commented Jun 14, 2022

Uh oh!

mzitnik commented Jun 25, 2022

Uh oh!

rernas35 commented Jun 25, 2022

Uh oh!

zhicwu commented Jun 26, 2022

Uh oh!

zhicwu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhicwu commented Jun 27, 2022

Uh oh!

rernas35 commented Jul 17, 2022

Uh oh!

rernas35 commented Jul 17, 2022

Uh oh!

rernas35 commented Jul 17, 2022

Uh oh!

zhicwu commented Sep 7, 2022

Uh oh!

zhicwu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhicwu commented Sep 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented May 2, 2022 •

edited

Loading

zhicwu commented May 2, 2022 •

edited

Loading

zhicwu left a comment •

edited

Loading