[feat][io] Upgarde ClickHouse driver to support loadbalance policy #18774
Conversation
Signed-off-by: tison <wander4096@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #18774 +/- ##
============================================
- Coverage 47.74% 46.43% -1.31%
+ Complexity 10623 10425 -198
============================================
Files 703 703
Lines 68828 68851 +23
Branches 7381 7382 +1
============================================
- Hits 32861 31974 -887
- Misses 32307 33276 +969
+ Partials 3660 3601 -59
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -38,7 +38,7 @@ | |||
| <version>${project.version}</version> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>ru.yandex.clickhouse</groupId> | |||
| <groupId>com.clickhouse</groupId> | |||
There was a problem hiding this comment.
Perhaps it's better to use shaded jar?
com.clickhouse clickhouse-jdbc 0.3.2-patch11 all * *There was a problem hiding this comment.
Thanks for your review @zhicwu!
What's the most common dependency conflict that can be avoided by using the shaded jar?
I can see using the basic JAR, the final connector NAR is in size 9.7M, and after switching to the shaded jar, it grows to 46M.
If no user reports the current solution's issue, I tend to keep it as is.
There was a problem hiding this comment.
What's the most common dependency conflict that can be avoided by using the shaded jar?
LZ4 and Gson. Put native library aside, if there's custom classloader for isolation, it should not be a problem.
I can see using the basic JAR, the final connector NAR is in size 9.7M, and after switching to the shaded jar, it grows to 46M.
Did you exclude all dependencies when specifying classifier? I'm not familiar with the Nifi maven plugin, but the JDBC driver with only http implementation is ~1MB. -all contains more like gRPC etc., so it's 20+ MB.
There was a problem hiding this comment.
Did you exclude all dependencies when specifying classifier?
No. I'm unsure what dependencies should be excluded XD.
There was a problem hiding this comment.
All of them because you only need the shaded jar :)
<dependency>
<groupId>com.clickhouse</groupId>
<artifactId>clickhouse-jdbc</artifactId>
<version>0.3.2-patch11</version>
<classifier>all</classifier>
<exclusions>
<exclusion>
<groupId>*</groupId>
<artifactId>*</artifactId>
</exclusion>
</exclusions>
</dependency>
|
Hi @tisonkun,
Having said that, since v0.3.2 is packed with both legacy and new JDBC drivers( Lastly, some more comments for you to consider regarding ClickHouse sink:
|
|
@zhicwu Thanks for your explanation! I think most of the usages of ClickHouse JDBC Driver are managed within the Pulsar IO framework, so the impact should also be managed. Still, it doesn't tell why we should use a shaded dependency here - what's the benefit? The NAR file size is a downsize, BTW. |
|
@nlu90 Agree. Test coverage is lack for this connector. I'm trying to add some. |
|
Hi @zhicwu! Let me ask some questions in details:
What's the behavior if we pass null to a non-nullable column previously?
Is there a CHANGELOG for the details how the mapping and handling change? |
Before v0.3.2, it depends on table structure(whether the column has default value) and server setting(whether
Unfortunately the answer is no. We use to have a CHANGELOG but it did not cover details like that. As I mentioned above, the impact to JDBC driver should be mainly about timestamp with timezone. @tisonkun & @nlu90, with no doubt that adding tests is needed. However, I'd suggest to start with acceptance tests from pulsar's point of view, instead of being distracted by compatibility issue of specific driver ;) |
|
@zhicwu Thanks for your explanation! I think it's good to merge now. Merging... @codelipenghui @eolivelli @Technoboy- FYI release note required on the next release. |
This closes #12998.
Documentation
May or may not we add some docs on this feature. I'm not sure.
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: