[SQLLINE-5] Use ServiceLoader to load drivers rather than forName #235
Conversation
snuyanzin
changed the title
| @@ -200,6 +201,7 @@ public String getVersion() { | |||
| * | |||
| * @see #DEFAULT_DRIVERS | |||
| */ | |||
| @Deprecated | |||
There was a problem hiding this comment.
@snuyanzin how list of known drivers can be overridden without using depicted method?
There was a problem hiding this comment.
Could you please clarify what you mean?
Before this PR known drivers used to filter classes from classpath and to load drivers if they are matched to known drivers.
After PR changes java service provider functionality [1] will be used, i.e. java could go through jars looking for META-INF/services/java.sql.Driver and if success then load the driver.
So each driver which has such file inside its jar becomes known. It will allow to add more drivers in classpath and load them automatically without code changes
[1] https://docs.oracle.com/javase/8/docs/api/java/util/ServiceLoader.html
There was a problem hiding this comment.
For Drill we were overriding all known drivers to only one - "org.apache.drill.jdbc.Driver" since Drill SqlLine is intended only for Apache Drill.
There was a problem hiding this comment.
Looks like for your case it is required to have limit which could be done via Application#initDrivers. What about logic like if Application#initDrivers is empty then any driver found via servicelocator is allowed. If Application#initDrivers is not empty then only drivers which are present in both Application#initDrivers and servicelocator are allowed.
However the name Application#initDrivers becomes not matching to what it started to do. So it makes sense to add a new method with more suitable name and leave it deprecated and made call to the new one as it is public.
There was a problem hiding this comment.
Sounds good, just indicate in Javadoc which new method should be used.
|
Don't represent "allow any driver" with empty collection. That causes a discontinuity going from 0 to 1 element. Use the null collection. How about using a List rather than a Collection? Some applications that have a strong preference of one driver over another, so order might be important. Please continue to add separate commits rather than squashing. I have already started reviewing the first commit, so separate follow-up commits will be easier for me to merge. |
…ny driver is allowed
|
Thank you for your comment. |
|
The two most recent commits have @snuyanzin's email address but use the name "Ekaterina". Do I need to preserve that authorship information or can I squash the commits? |
|
:) it can be squashed |
The PR implements usage of drivers mentioned by
META-INF/services/java.sql.DriverAlso it allows to see errors in case of drivers loading which is impossible for current version.
Checked with latest drivers
fixes #5