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

JDBC Binding does not support multiple host for failover #1184

Closed
seybi87 opened this issue Jul 11, 2018 · 5 comments
Closed

JDBC Binding does not support multiple host for failover #1184

seybi87 opened this issue Jul 11, 2018 · 5 comments

Comments

@seybi87
Copy link
Contributor

seybi87 commented Jul 11, 2018

Hi guys,
I was trying to benchmark CockroachDB (using the JDBC binding and the PostgreSQL JDBC connector) with respect to node failures and I came across the following issue:

JDBC support multiple host in the connection String to enable failover, e.g.:
jdbc:postgresql://host1:port1,host2:port2/database

Yet, the current JDBC-binding epects multiple hosts only in the following form:
jdbc:postgresql://host1:port1/database,jdbc:postgresql://host2:port2/database

And as the provided connection String is split by "," to create multiple connections (check the respsective line 198 JDBC-Binding ) this will result in an error for the failover connection String with multiple hosts.

Hence, the splitting identifier at line 198 should be changed to sth. which not conflicts with JDBC connection string chars.

I did a simple test by replacing "," with ";" and it worked without a problem for a failover connection String.

Would be great if you can apply this fix for upcoming releases

Cheers,
Daniel

@busbey
Copy link
Collaborator

busbey commented Jul 11, 2018

Hi Daniel! do you have the time to put together a PR?

@seybi87
Copy link
Contributor Author

seybi87 commented Jul 12, 2018

Yes, I will try to do it within the next days.

@busbey
Copy link
Collaborator

busbey commented Aug 21, 2018

Any progress?

@seybi87
Copy link
Contributor Author

seybi87 commented Sep 13, 2018

Sorry for coming back to this quite late.

Just submitted a pull request to fix this issue

@busbey
Copy link
Collaborator

busbey commented Sep 8, 2019

#1233 has been merged.

@busbey busbey closed this as completed Sep 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants