[ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >=0.180 #3694
Conversation
zjffdu
force-pushed
the
ZEPPELIN-2891
branch
2 times, most recently
from
3c3652a to
3ed1483
Compare
| } catch (ClassNotFoundException e) { | ||
| clazz = Class.forName("io.prestosql.jdbc.ConnectionProperties"); | ||
| } | ||
| Method lookUpKeyMethod = clazz.getMethod("forKey", String.class); |
There was a problem hiding this 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.)
There was a problem hiding this comment.
Thanks for the comment , will address it
There was a problem hiding this 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.)
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.
There was a problem hiding this 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.
There was a problem hiding this comment.
Thanks for your conform
zjffdu
force-pushed
the
ZEPPELIN-2891
branch
2 times, most recently
from
ecd7257 to
3466f5b
Compare
…0.180 ### 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 Author: Jeff Zhang <[email protected]> Closes #3694 from zjffdu/ZEPPELIN-2891 and squashes the following commits: 2a77ae2 [Jeff Zhang] [ZEPPELIN-2891]. Impossible to use jdbc interface with presto-jdbc >= 0.180
|
A PR is already submitted. #3712 |
3 participants
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
What is the Jira issue?
How should this be tested?
Screenshots (if appropriate)
Questions: