amazon-archives / sql-jdbc

🔍 Open Distro for Elasticsearch JDBC Driver
Apache License 2.0
111 stars 49 forks source link

should setReadonly method throw Exception? #19

Closed penghuiy closed 5 years ago

penghuiy commented 5 years ago

I use Sharding-proxy(use HikariPool), But I have no way to explicitly specify database isReadOnly,at least for now, resulting in a SQLNonTransientException ("read-only mode can not be disabled."), causing the connection to fail. I noticed that other similar jdbc drives the setReadOnly method are not will throw exception, such as

sql-server(https://github.com/microsoft/mssql-jdbc/blob/dev/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java),

H2(https://github.com/h2database/h2database/blob/master/h2/src/main/org/h2/jdbc/JdbcConnection.java),

I don't know how to solve it

dai-chen commented 5 years ago

Thanks for reporting! The exception thrown seems by design: https://github.com/opendistro-for-elasticsearch/sql-jdbc/blob/625614647c5110daa4e524143d55ecabb8e50794/src/main/java/com/amazon/opendistroforelasticsearch/jdbc/ConnectionImpl.java#L200

JDBC spec says: "Puts this connection in read-only mode as a hint to the driver to enable database optimizations." I feel this is not applicable in driver backed by ElasticSearch.

So I wonder what's your use case? Is your Sharding-proxy trying to set this anyway?

penghuiy commented 5 years ago

I want to provide multiple users with an interface that can perform sql queries(mysql-proxy), and route queries to different indexes or database tables according to the userid. Sharding-proxy datasource uses hikari pool, I have found a discussion about the hikari pool how to setReadOnly(https://github.com/brettwooldridge/HikariCP/issues/1116). if don't explicitly specify isReadOnly, default it will call   Jdbc's setReadOnly method.currently, Sharding-proxy can't configure datasource isReadonly, so I will receive an exception. So I hope to find some solutions.

dai-chen commented 5 years ago

I want to provide multiple users with an interface that can perform sql queries(mysql-proxy), and route queries to different indexes or database tables according to the userid. Sharding-proxy datasource uses hikari pool, I have found a discussion about the hikari pool how to setReadOnly(brettwooldridge/HikariCP#1116). if don't explicitly specify isReadOnly, default it will call   Jdbc's setReadOnly method.currently, Sharding-proxy can't configure datasource isReadonly, so I will receive an exception. So I hope to find some solutions.

Thanks for the information! Need to investigate if any action could be taken when setReadOnly invoked? If not, probably simply removing the exception thrown in our code would be the proper way to fix the issue.

penghuiy commented 5 years ago

I have created an issue for Sharding-proxy,and received feedback. Maybe, it’s more reasonable to explicitly specify a datasource readonly. I will wait few days

dai-chen commented 5 years ago

I have created an issue for Sharding-proxy,and received feedback. Maybe, it’s more reasonable to explicitly specify a datasource readonly. I will wait few days

Thanks for letting us know! Will evaluate it from our end too.

dai-chen commented 5 years ago

@penghuiy Any response from Sharding-proxy team? It's easy to fix by removing the throw exception code from our end. But I feel it may not be the right way. Because I assume the caller expects something to happen after invoking setReadOnly().

So I think either avoid calling setReadOnly or add behavior for it accordingly in our driver rather than disregard it may be more proper way to fix the issue.

penghuiy commented 5 years ago

@penghuiy Any response from Sharding-proxy team? It's easy to fix by removing the throw exception code from our end. But I feel it may not be the right way. Because I assume the caller expects something to happen after invoking setReadOnly().

So I think either avoid calling setReadOnly or add behavior for it accordingly in our driver rather than disregard it may be more proper way to fix the issue.

Right,I agree with you. the PR is awaiting review,I believe that there will be results soon.

dai-chen commented 5 years ago

@penghuiy Any response from Sharding-proxy team? It's easy to fix by removing the throw exception code from our end. But I feel it may not be the right way. Because I assume the caller expects something to happen after invoking setReadOnly(). So I think either avoid calling setReadOnly or add behavior for it accordingly in our driver rather than disregard it may be more proper way to fix the issue.

Right,I agree with you. the PR is awaiting review,I believe that there will be results soon.

Thanks! Please keep us updated.

penghuiy commented 5 years ago

@dai-chen The PR has been merged, Thank you for your advice and help.

dai-chen commented 5 years ago

@dai-chen The PR has been merged, Thank you for your advice and help.

You're welcome! Thanks for the update!