Generic path to create specific SqlConnectOptions

[aguibert]

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);

[vietj]

Rather name than kind

[aguibert]

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)

[vietj]

I believe the correct naming to use here is rdbms or dbms

[aguibert]

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)?

[vietj]

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 ***@***.***> 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 <#644 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABXDCVA4M5RLMNOQMCYZJDRTPHTBANCNFSM4NFJBIQA>.

[aguibert]

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)

[BillyYccc]

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.

[BillyYccc]

DatabaseProtocol is not a good choice because not all DB protocols are named by the database name such as TDS protocol for MSSQL Server.

[vietj]

two DBs might use the same protocol

…

On 26 May 2020, at 18:14, Billy Yuan ***@***.***> 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 <#644 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABXDCRC2TWPT2GZ5PE4QLDRTPTIBANCNFSM4NFJBIQA>.

[aguibert]

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:

1. Multiple drivers are on the classpath
2. 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)

[vietj]

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 ***@***.***> 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 <#644 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABXDCWMSYMIBZLW4VOENA3RTPTD5ANCNFSM4NFJBIQA>.

[vietj]

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 ***@***.***> 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 <#644 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABXDCQL7XL4RGSDCTGVT73RTPTTPANCNFSM4NFJBIQA>.

[aguibert]

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.

[vietj]

@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))

[vietj]

It seems that using plain SqlConnectOptions leads to this issue #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.

[aguibert]

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?

[vietj]

On 27 May 2020, at 16:42, Andrew Guibert ***@***.***> 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 <#644 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABXDCVAR7FNKPL7LHCXWJ3RTURDRANCNFSM4NFJBIQA>.

[aguibert]

@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);

[vietj]

this creates an implicit dependency (yet weak) from sql-client onto the existing implementations. We are actually going to have a JDBC implementation as well that will not be in this GitHub repository, so I think it would be best better to avoid this and have instead each driver define a string constant for this.

[aguibert]

When we add a JDBC implementation in a separate repo we can add another value to this enum, right?
Any other drivers that come along will still work, and will ultimately be able to define what they want their string constants to be.
Mainly I'm just going for user-convenience here by adding "knowledge of" the known implementations, I don't know if you could really call it a dependency

[vietj]

there isn't much a point of having KnownDrivers enum than having a glorified factory, i.e

if you write

SqlConnectOptions options = Driver.createConnectOptions(Driver.KnownDrivers.POSTGRESQL)

then you can simply write

SqlConnectOptions options = new PgConnectOptions();

[aguibert]

I was mainly looking at this from a UX perspective, but since this is just the SPI path (not API) UX is less important because users won't be doing this directly.
I'll go ahead and remove the KnownDrivers enum and associated code

[aguibert]

Any further comments on this one @vietj or are we good to merge?

[BillyYccc]

We need to add VertxGen annotation to make it work for ReactiveX/etc...

[BillyYccc]

can we remove these @GenIgnore annotations ?

[aguibert]

The build was failing saying these were not supported, I can go back and check again, but the whole generation step is an enigma to me

[BillyYccc]

could you push the branch to the upstream repo so that we could try to build that and see what happens?