eclipse-vertx
Generic path to create specific SqlConnectOptions
This improves the driver API so that a specific DB type can be requested using the generic SqlConnectOptions path. This is necessary for situations when A) user does not want a direct dependency on the driver API and B) there may be multiple different drivers on the classpath.
Example usages:
// Request DB2 options specifically by `Driver.KnownDrivers` enum
SqlConnectOptions a = Driver.createOptions(KnownDrivers.DB2);
// Request DB2 options specifically by string driver name
SqlConnectOptions a = Driver.createOptions("DB2");
// Request any available options. If != 1 drivers are found, an error is raised
SqlConnectOptions a = Driver.createOptions(KnownDrivers.ANY_DRIVER);
I think name is a bit too ambiguous -- I think people will confuse it with the DB name (.setDatabase(String databaseName)
I chose kind because that is also what Quarkus uses for this sort of setting: https://quarkus.io/guides/all-config#quarkus-datasource_quarkus.datasource.db-kind
Some other options to consider:
-
setDatabaseKind(String) -
setDatabaseType(String)
So @vietj you are suggesting setRDBMS(String)? To me that doesn't imply that we are just setting the type name of the DB. Also, I'd like to keep longer acronyms out of the API.
What about setDatabaseDistributionName(String) or setDatabaseVendorName(String)?
vendor name is different, one vendor can have several databases.
DBMS looks very fine to me and something that everybody understand as it has been around for a few decades.
On 26 May 2020, at 16:35, Andrew Guibert [email protected] wrote:
So @vietj https://github.com/vietj you are suggesting setRDBMS(String)? To me that doesn't imply that we are just setting the type name of the DB. Also, I'd like to keep longer acronyms out of the API.
What about setDatabaseDistributionName(String) or setDatabaseVendorName(String)?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eclipse-vertx/vertx-sql-client/pull/644#issuecomment-634064811, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCVA4M5RLMNOQMCYZJDRTPHTBANCNFSM4NFJBIQA.
To me setDBMS still seems ambiguious. I would almost expect it to accept a composite object that represents the entire backend DB, not just the type of DB.
How about setDatabaseProtocol(String)?
@BillyYccc any opinion on this one? To recap, the options currently proposed are:
-
setKind(String) -
setDatabaseKind(String) -
setDatabaseType(String) -
setDatabase(String) -
setDBMS(String) -
setRDBMS(String) -
setDatabaseDistributionName(String) -
setDatabaseProtocol(String)
How about simply driverName? The database client indeed does not care about what the database server is as long as they're able to communicate with each other via a specific protocol. For example you can use PG client to connect with Postgres or other databases like Cockroach.
DatabaseProtocol is not a good choice because not all DB protocols are named by the database name such as TDS protocol for MSSQL Server.
two DBs might use the same protocol
On 26 May 2020, at 18:14, Billy Yuan [email protected] wrote:
DatabaseProtocol is not a good choice because not all DB protocols are named by the database name such as TDS protocol for MSSQL Server.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eclipse-vertx/vertx-sql-client/pull/644#issuecomment-634125703, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCRC2TWPT2GZ5PE4QLDRTPTIBANCNFSM4NFJBIQA.
Sure, I like setDriverName(String).
BTW, the reason I am trying to add this in the first place is so that a user can optionally specify the database type for situations where:
- Multiple drivers are on the classpath
- Generic properties config are used (i.e. host/port/user/etc)
We need a way to disambiguate which driver to select (without having a direct dependency on the driver-specific API)
given that the SPI for this is named Driver, it seems that using "driverName" or "driver" is a good fit.
On 26 May 2020, at 18:13, Billy Yuan [email protected] wrote:
How about simply driverName? The database client indeed does not care about what the database server is as long as they're able to communicate with each other via a specific protocol. For example you can use PG client to connect with Postgres or other databases like Cockroach.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eclipse-vertx/vertx-sql-client/pull/644#issuecomment-634125091, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCWMSYMIBZLW4VOENA3RTPTD5ANCNFSM4NFJBIQA.
I think actually that an overload with the driverName would work better.
static Pool pool(String driverName, SqlConnect options) { ... }
The SqlConnectOptions describe how to connect to the database (i.e coordinates) and not to which database it should be connected
On 26 May 2020, at 18:17, Andrew Guibert [email protected] wrote:
Sure, I like setDriverName(String).
BTW, the reason I am trying to add this in the first place is so that a user can optionally specify the database type for situations where:
Multiple drivers are on the classpath Generic properties config are used (i.e. host/port/user/etc) We need a way to disambiguate which driver to select (without having a direct dependency on the driver-specific API)
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eclipse-vertx/vertx-sql-client/pull/644#issuecomment-634127416, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCQL7XL4RGSDCTGVT73RTPTTPANCNFSM4NFJBIQA.
Ok, sounds like we all agree on driverName 👍
As for where to specify it, SqlConnectOptions currently describe how to connect to the DB, but I don't think it would be much of a stretch to also include driverName in here, especially given that it's an optional setting.
Compared with the tradeoff of overloading Pool#pool, I think adding it to SqlConnectOptions is better, because if we add another overload combination we would go from 3 pool() methods to 6 pool() methods and I think it's confusing to users to have too many overload options.
@aguibert I agree for the overloading, another way would be to have a static method that creates SqlConnectOptions for a given name, i.e Pool.create(Driver.connectOptions("mysql").setPort(1234)....., poolOptions))
It seems that using plain SqlConnectOptions leads to this issue https://github.com/eclipse-vertx/vertx-sql-client/issues/660 .
In order to fix the driver name and the reported issue, we need to have SqlConnectOptions an abstract class as it used to be and introduce a static factory method in io.vertx.sqlclient.spi.Driver that will create the right subclass according to the driver name that is wanted.
This will solve this PR and the issue #660 issue.
If we add the static method for Driver.connectOptions("mysql"), does that imply we would want to turn SqlConnectOptions back into an abstract class then? Otherwise we're not solving the issue for all cases. However, this would be a breaking change that we couldn't make until 4.0.
I suppose we could add the new Driver.connectOptions now, and then turn SqlConnectOptions abstract in 4.0?
On 27 May 2020, at 16:42, Andrew Guibert [email protected] wrote:
If we add the static method for Driver.connectOptions("mysql"), does that imply we would want to turn SqlConnectOptions back into an abstract class then? Otherwise we're not solving the issue for all cases. However, this would be a breaking change that we couldn't make until 4.0.
I suppose we could add the new Driver.connectOptions now, and then turn SqlConnectOptions abstract in 4.0?
yes that would work great
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eclipse-vertx/vertx-sql-client/pull/644#issuecomment-634708152, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCVAR7FNKPL7LHCXWJ3RTURDRANCNFSM4NFJBIQA.
@vietj ok, I've reworked the PR based on your feedback. I removed the kind property from SqlConnectOptions and added a static Driver#createConnectOptions method. New usage looks like this:
SqlConnectOptions opts = Driver.createConnectOptions(KnownDrivers.DB2);
Pool pool = Pool.pool(opts);