You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2020/03/19 13:20:50 UTC
[GitHub] [zeppelin] zjffdu opened a new pull request #3694: [ZEPPELIN-2891].
Impossible to use jdbc interface with presto-jdbc >=0.180
zjffdu opened a new pull request #3694: [ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
URL: https://github.com/apache/zeppelin/pull/3694
### What is this PR for?
This PR is to fix the issue of use presto via jdbc interpreter. In this PR, I would only add properties that is valid for Presto jdbc driver. I tested it manully on the 2 versions of presto: prestodb, prestosql.
### What type of PR is it?
[Bug Fix ]
### Todos
* [ ] - Task
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-2891
### How should this be tested?
* CI pass and manually tested on prestodb and prestosql.
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zeppelin] zjffdu commented on a change in pull request #3694:
[ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3694: [ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
URL: https://github.com/apache/zeppelin/pull/3694#discussion_r395441410
##########
File path: jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java
##########
@@ -407,7 +408,33 @@ private void setUserProperty(String propertyKey, InterpreterContext interpreterC
}
private void createConnectionPool(String url, String user, String propertyKey,
- Properties properties) throws SQLException, ClassNotFoundException {
+ Properties properties) throws SQLException, ClassNotFoundException, IOException {
+
+ String driverClass = properties.getProperty(DRIVER_KEY);
+ if (driverClass != null && (driverClass.equals("com.facebook.presto.jdbc.PrestoDriver")
+ || driverClass.equals("io.prestosql.jdbc.PrestoDriver"))) {
+ Properties newProperties = new Properties();
+ // Only add valid properties otherwise presto won't work.
+ try {
+ Class clazz = null;
+ try {
+ clazz = Class.forName("com.facebook.presto.jdbc.ConnectionProperties");
+ } catch (ClassNotFoundException e) {
+ clazz = Class.forName("io.prestosql.jdbc.ConnectionProperties");
+ }
+ Method lookUpKeyMethod = clazz.getMethod("forKey", String.class);
Review comment:
Thanks for your conform
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zeppelin] electrum commented on a change in pull request #3694:
[ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
Posted by GitBox <gi...@apache.org>.
electrum commented on a change in pull request #3694: [ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
URL: https://github.com/apache/zeppelin/pull/3694#discussion_r395161618
##########
File path: jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java
##########
@@ -407,7 +408,33 @@ private void setUserProperty(String propertyKey, InterpreterContext interpreterC
}
private void createConnectionPool(String url, String user, String propertyKey,
- Properties properties) throws SQLException, ClassNotFoundException {
+ Properties properties) throws SQLException, ClassNotFoundException, IOException {
+
+ String driverClass = properties.getProperty(DRIVER_KEY);
+ if (driverClass != null && (driverClass.equals("com.facebook.presto.jdbc.PrestoDriver")
+ || driverClass.equals("io.prestosql.jdbc.PrestoDriver"))) {
+ Properties newProperties = new Properties();
+ // Only add valid properties otherwise presto won't work.
+ try {
+ Class clazz = null;
+ try {
+ clazz = Class.forName("com.facebook.presto.jdbc.ConnectionProperties");
+ } catch (ClassNotFoundException e) {
+ clazz = Class.forName("io.prestosql.jdbc.ConnectionProperties");
+ }
+ Method lookUpKeyMethod = clazz.getMethod("forKey", String.class);
Review comment:
Please don’t access internals of the driver with reflection. We can change this at any time in the future which will cause this code to break.
You can find the list of supported properties here: https://prestosql.io/docs/current/installation/jdbc.html
The important thing is to not add random properties like “url”. Zeppelin should not be adding properties unless it knows they are valid or they come from an end user. (If the user adds an invalid property, that’s on the user, and we don’t need to protect them from themselves.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zeppelin] electrum commented on a change in pull request #3694:
[ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
Posted by GitBox <gi...@apache.org>.
electrum commented on a change in pull request #3694: [ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
URL: https://github.com/apache/zeppelin/pull/3694#discussion_r395433276
##########
File path: jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java
##########
@@ -407,7 +408,33 @@ private void setUserProperty(String propertyKey, InterpreterContext interpreterC
}
private void createConnectionPool(String url, String user, String propertyKey,
- Properties properties) throws SQLException, ClassNotFoundException {
+ Properties properties) throws SQLException, ClassNotFoundException, IOException {
+
+ String driverClass = properties.getProperty(DRIVER_KEY);
+ if (driverClass != null && (driverClass.equals("com.facebook.presto.jdbc.PrestoDriver")
+ || driverClass.equals("io.prestosql.jdbc.PrestoDriver"))) {
+ Properties newProperties = new Properties();
+ // Only add valid properties otherwise presto won't work.
+ try {
+ Class clazz = null;
+ try {
+ clazz = Class.forName("com.facebook.presto.jdbc.ConnectionProperties");
+ } catch (ClassNotFoundException e) {
+ clazz = Class.forName("io.prestosql.jdbc.ConnectionProperties");
+ }
+ Method lookUpKeyMethod = clazz.getMethod("forKey", String.class);
Review comment:
I don’t foresee us removing properties, as we take backwards compatibility seriously. I believe the main issue here was not validation of arbitrary properties, but that Zeppelin was adding its own properties like “url”. If we can avoid that, then we shouldn’t need to worry about validation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zeppelin] qiguoxiaosheng commented on issue #3694:
[ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
Posted by GitBox <gi...@apache.org>.
qiguoxiaosheng commented on issue #3694: [ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
URL: https://github.com/apache/zeppelin/pull/3694#issuecomment-607778406
A PR is already submitted. #3712
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zeppelin] zjffdu commented on a change in pull request #3694:
[ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3694: [ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
URL: https://github.com/apache/zeppelin/pull/3694#discussion_r395424593
##########
File path: jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java
##########
@@ -407,7 +408,33 @@ private void setUserProperty(String propertyKey, InterpreterContext interpreterC
}
private void createConnectionPool(String url, String user, String propertyKey,
- Properties properties) throws SQLException, ClassNotFoundException {
+ Properties properties) throws SQLException, ClassNotFoundException, IOException {
+
+ String driverClass = properties.getProperty(DRIVER_KEY);
+ if (driverClass != null && (driverClass.equals("com.facebook.presto.jdbc.PrestoDriver")
+ || driverClass.equals("io.prestosql.jdbc.PrestoDriver"))) {
+ Properties newProperties = new Properties();
+ // Only add valid properties otherwise presto won't work.
+ try {
+ Class clazz = null;
+ try {
+ clazz = Class.forName("com.facebook.presto.jdbc.ConnectionProperties");
+ } catch (ClassNotFoundException e) {
+ clazz = Class.forName("io.prestosql.jdbc.ConnectionProperties");
+ }
+ Method lookUpKeyMethod = clazz.getMethod("forKey", String.class);
Review comment:
@elec
> Please don’t access internals of the driver with reflection. We can change this at any time in the future which will cause this code to break.
>
> You can find the list of supported properties here: https://prestosql.io/docs/current/installation/jdbc.html
>
> The important thing is to not add random properties like “url”. Zeppelin should not be adding properties unless it knows they are valid or they come from an end user. (If the user adds an invalid property, that’s on the user, and we don’t need to protect them from themselves.)
One concern I have is whether presto would change these valid properties. Because we would like to support multiple versions of presto. So if presto introduce new properties or remove some existing properties, that would be a problem for us.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zeppelin] asfgit closed pull request #3694: [ZEPPELIN-2891].
Impossible to use jdbc interface with presto-jdbc >=0.180
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3694: [ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
URL: https://github.com/apache/zeppelin/pull/3694
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zeppelin] qiguoxiaosheng removed a comment on issue #3694:
[ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
Posted by GitBox <gi...@apache.org>.
qiguoxiaosheng removed a comment on issue #3694: [ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
URL: https://github.com/apache/zeppelin/pull/3694#issuecomment-607777808
A PR is already submitted. #3712
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zeppelin] zjffdu commented on a change in pull request #3694:
[ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3694: [ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
URL: https://github.com/apache/zeppelin/pull/3694#discussion_r395368306
##########
File path: jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java
##########
@@ -407,7 +408,33 @@ private void setUserProperty(String propertyKey, InterpreterContext interpreterC
}
private void createConnectionPool(String url, String user, String propertyKey,
- Properties properties) throws SQLException, ClassNotFoundException {
+ Properties properties) throws SQLException, ClassNotFoundException, IOException {
+
+ String driverClass = properties.getProperty(DRIVER_KEY);
+ if (driverClass != null && (driverClass.equals("com.facebook.presto.jdbc.PrestoDriver")
+ || driverClass.equals("io.prestosql.jdbc.PrestoDriver"))) {
+ Properties newProperties = new Properties();
+ // Only add valid properties otherwise presto won't work.
+ try {
+ Class clazz = null;
+ try {
+ clazz = Class.forName("com.facebook.presto.jdbc.ConnectionProperties");
+ } catch (ClassNotFoundException e) {
+ clazz = Class.forName("io.prestosql.jdbc.ConnectionProperties");
+ }
+ Method lookUpKeyMethod = clazz.getMethod("forKey", String.class);
Review comment:
Thanks for the comment , will address it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zeppelin] qiguoxiaosheng commented on issue #3694:
[ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
Posted by GitBox <gi...@apache.org>.
qiguoxiaosheng commented on issue #3694: [ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180
URL: https://github.com/apache/zeppelin/pull/3694#issuecomment-607777808
A PR is already submitted. #3712
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services