-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
upgrade clickhouse jdbc driver 0.3.2-patch10 #24424
Conversation
Assigning reviewers. If you would like to opt out of this review, comment R: @apilloud for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Codecov Report
@@ Coverage Diff @@
## master #24424 +/- ##
==========================================
- Coverage 73.46% 73.28% -0.19%
==========================================
Files 714 723 +9
Lines 96497 98227 +1730
==========================================
+ Hits 70896 71981 +1085
- Misses 24279 24924 +645
Partials 1322 1322
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -499,6 +499,7 @@ class BeamModulePlugin implements Plugin<Project> { | |||
def spark3_version = "3.1.2" | |||
def spotbugs_version = "4.0.6" | |||
def testcontainers_version = "1.16.3" | |||
def testcontainers_version_clickhouse = "1.17.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason to not just update the testcontainers version globally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johnjcasey Yes i can, just was not sure that it is OK i will do that and push the change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johnjcasey I have pushed the change testcontainers version
@@ -49,7 +49,7 @@ idea { | |||
} | |||
} | |||
|
|||
def clickhouse_jdbc_version = "0.2.6" | |||
def clickhouse_jdbc_version = "0.3.2-patch10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not to the latest version (0.3.2-patch11
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cause some conflict with testcontainers_version need to have deeper look on that,but for now lets continue with 0.3.2-patch10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
// try { | ||
String value = str; | ||
/* | ||
String value = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be removed?
} | ||
} | ||
} | ||
new ClickHouseContainer("clickhouse/clickhouse-server:22.5") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we test against the latest CH version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to 22.9
} | ||
|
||
@AfterClass | ||
public static void tearDown() { | ||
clickHouse.close(); | ||
zookeeper.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we close
it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@johnjcasey can you help with the PR |
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/ClickHouseWriter.java
Show resolved
Hide resolved
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/ClickHouseWriter.java
Show resolved
Hide resolved
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/ClickHouseWriter.java
Outdated
Show resolved
Hide resolved
sdks/java/io/clickhouse/src/test/java/org/apache/beam/sdk/io/clickhouse/AtomicInsertTest.java
Show resolved
Hide resolved
sdks/java/io/clickhouse/src/test/java/org/apache/beam/sdk/io/clickhouse/AtomicInsertTest.java
Outdated
Show resolved
Hide resolved
sdks/java/io/clickhouse/src/test/java/org/apache/beam/sdk/io/clickhouse/AtomicInsertTest.java
Show resolved
Hide resolved
sdks/java/io/clickhouse/src/test/java/org/apache/beam/sdk/io/clickhouse/TableSchemaTest.java
Outdated
Show resolved
Hide resolved
sdks/java/io/clickhouse/src/test/java/org/apache/beam/sdk/io/clickhouse/TableSchemaTest.java
Outdated
Show resolved
Hide resolved
Reminder, please take a look at this pr: @apilloud @Abacn @johnjcasey |
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @kileys for label java. Available commands:
|
stop reviewer notifications |
@johnjcasey has started this review and probably is the right person to complete it. I imagine responses may be slower since we are in the midst of the holiday season |
Stopping reviewer notifications for this pull request: requested by reviewer |
run java precommit |
run java_pulsar_io_direct precommit |
@johnjcasey do we know on what Apache Beam version it will be merged? |
it will be 2.45 |
@johnjcasey is there ETA for v2.45 release to communicate to our customers? |
The cut was last week, and we are closing out release blockers now. It could be as far away as 5 weeks from now, but more likely sooner |
Upgrade clickhouse jdbc driver to 0.3.2-patch10
@methodmissing
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username
).addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.