Extend JDBC URL pattern to support failover #411
Conversation
Codecov Report
@@ Coverage Diff @@
## master #411 +/- ##
============================================
+ Coverage 62.60% 62.99% +0.39%
- Complexity 1256 1263 +7
============================================
Files 135 135
Lines 6662 6662
Branches 519 519
============================================
+ Hits 4171 4197 +26
+ Misses 2216 2197 -19
+ Partials 275 268 -7
Continue to review full report at Codecov.
|
|
LGTM |
| public static final String HOST_DELIMITER = ","; | ||
| public static final String PORT_DELIMITER = ":"; | ||
|
|
||
| public static final Pattern CONNECTION_PATTERN = Pattern.compile("//(?<hosts>[^/?\\s]+)(?:/(?<database>([a-zA-Z0-9_]+)))?(?:\\?(?<query>.*))?"); |
There was a problem hiding this comment.
How does ck1,ck2:1234 be interpreted?
ck1:default_portandck2:1234orck1:1234andck2:1234
There was a problem hiding this comment.
Any behavior looks reasonable, please just add clear comments here.
There was a problem hiding this comment.
The query is confusing, we can learn something from MySQL[1] or Apache Hive[2], maybe vars or properties is a proper name.
[1] https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-reference-jdbc-url-format.html
[2] https://cwiki.apache.org/confluence/display/hive/hiveserver2+clients#HiveServer2Clients-ConnectionURLs
There was a problem hiding this comment.
Some bad but recoverable patterns ck1:, ,ck1, ck1,ck2,, ck1:,ck2
There was a problem hiding this comment.
Another bad but recoverable pattern jdbc:clickhouse://ck1,ck2/
| lastException = e; | ||
| } | ||
| tryIndex++; | ||
| } while (nativeClient == null && tryIndex < configure.hosts().size()); |
There was a problem hiding this comment.
Looks like the former node always has high priority, so it does not a load-balanced implementation? If yes, let's add clear comments on the regex pattern to avoid surprising users.
There was a problem hiding this comment.
it does not a load-balanced implementation
...-native-jdbc/src/test/java/com/github/housepower/jdbc/FailoverClickhouseConnectionITest.java
Show resolved
Hide resolved
...-native-jdbc/src/test/java/com/github/housepower/jdbc/FailoverClickhouseConnectionITest.java
Outdated
Show resolved
Hide resolved
| "//,ck1", | ||
| "//ck1,ck2,", | ||
| "//ck1:,ck2", | ||
| "//ck1,ck2/" |
There was a problem hiding this comment.
I have no objection to treating them illegal except the last one, even though I think they are recoverable.
Can we make jdbc:clickhouse//ck1,ck2/ acceptable?
There was a problem hiding this comment.
It's not a big deal, as long as document it clearly.
|
Thanks a lot to @loneylee for bringing this amazing feature, I updated the PR description to make it clear, and please double-check if I misunderstand something. |
|
I am honored to be able to help the community, and if there are issues related to this issue in the future, I am willing to assist in solving it |
|
LGTM, Let's merge ! |
* support failover * fix pattern and add ut * fix ut wrong pair host and port
* support failover * fix pattern and add ut * fix ut wrong pair host and port
This PR proposes to extend the JDBC URL pattern as follows to support failover.
jdbc:clickhouse://host1:port1,host2:port2[/database][?prop1=value1&prop2=value2]To be clear, do not expect it to act as load balancing, the mechanism of failover is: