Thanks. I considered this approach but discounted it for a few reasons:

1. It increases the size;
2. It forces a particular JDBC driver version which is difficult for the users to replace/substitute;
3. It includes drivers which have licenses which are not compatible with the license of the main project;

I've backported a solution for my 3.x customers that allows them to include the JDBC driver externally via a workaround and some additional code, but I am not adding new features to 3.x upstream at this point.

This will be fixed in the upstream properly with Apiman 4 (probably).

The cache change is useful though by itself.

Maybe a compromise is just to include the Postgres driver for now, but I'm still wary about compatibility issues and forcing a particular driver version.

Passing run #957 ↗︎

0

17

0

0

0

Details:

_{This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.}

Thanks for the feedback Marc!
I already thought that the drivers are problematic for the upstream, I just copied them over from the wildfly pom to test everything.

The main problem we encountered was the client that was created again and again.
So the cache was useful in our case. I just tried to use the same libs you used in other places for this.

Thanks for the feedback Marc! I already thought that the drivers are problematic for the upstream, I just copied them over from the wildfly pom to test everything.

The main problem we encountered was the client that was created again and again. So the cache was useful in our case. I just tried to use the same libs you used in other places for this.

Yeah, the way we get away with it is that we never distribute the project or code with those license-incompatible drivers. They are referenced only for test or development purposes such as generating DDL, etc.

I'm probably going to drop some of them for upstream anyway, which should hopefully fix that issue entirely.

Yet more things for me to consider in Apiman 4 I suppose. I'm not getting to spend as much time on it as I would like, but still trying to push ahead.

OK, let's split this topic.

I'll commit the cache enhancement separately, and then we can think separately about whether we should include any drivers in 3.x or just wait for Apiman 4 to avoid complicating things.

I've thought of a potential solution that's even better than the workaround I use for customers to allow an external JDBC driver, so I'll experiment with that later when I have some time 🤞. Posting this as a reminder to myself.

OK, let's split this topic.

I'll commit the cache enhancement separately, and then we can think separately about whether we should include any drivers in 3.x or just wait for Apiman 4 to avoid complicating things.

I think that is a good idea. I can remove the drives from my commit if you want?

Whichever you prefer, sounds good to me 🙏

I'm also trying to get rid of the old ES libraries that were dragged in during a contribution a few years ago which has massively bulked out the gateway. That's a WIP for Apiman 4 (usual caveats apply for time and money, etc).

Whichever you prefer, sounds good to me pray

Okay I will do it and directly add the changelog here as well :)

Okay, related thought... Is there any reason not to use a Vert.x shared connection pool any more? Can we generate a key for the Vert.x shared connection instead of caching our own?

Thinking out loud here.

For example, if we are not sure we can reliably create a key that will guarantee we don't mix up the wrong clients (which could be a problem with this enhancement anyway I guess).

Also, should users of the JDBC client not be closing the connection themselves? Do we not make that available to them.

Mhh you may be right about that, my vert.x knowledge is not that deep.
We could do it in the same way like we do it here:

jdbcClient = JDBCClient.createShared(vertx, parseConfig(config), dsName);

I was not totally sure if we really need a new instance of the client all the time:

Subsequent calls will return a new client instance that uses the same data source, so the configuration won’t be used.
https://vertx.io/docs/3.9.16/vertx-jdbc-client/java/#_using_default_shared_data_source

Also, should users of the JDBC client not be closing the connection themselves? Do we not make that available to them.

Currently I cannot see in the logs that a client is close.
Only when the close is called I see the following log:

 HikariDataSource - HikariPool-1 - Shutdown completed.

Yeah, this requires a careful look because if you get the cache key wrong, then you may end up mixing together unrelated SQL calls and that will break things in extremely confusing ways.

I recall I avoided implementing caching at the time for that reason. That's why the comment references "jsonConfigs" as I was trying to figure out what needed to be included in the cache key to guarantee uniqueness.

It might actually be easier to change the code calling this impl, not sure...

I will try to look soon.

I checked that as well and it seems to work as well.
The only things is that we would also need to provide a unique key (even for the shared).

Without a dsName it uses a default name (which than would return the same client for all configs)

static JDBCClient createShared(Vertx vertx, JsonObject config) {
        return new JDBCClientImpl(vertx, config, "DEFAULT_DS");
}

Question - why is the calling code not setting a dsName of their own, to ensure it is shared. From a quick review, it seems we're just doing that out of precaution that we're not sure what the correct cache key is (same problem again!).

Couple of thoughts:

Wonder if we could just set a hashcode and equals function in JdbcOptionsBean and use that as the safest possible cache key, and always use a shared client? We'd need to ensure we account for unlikely hashcode collisions in our naming I suppose, but we could do something like...

Map<JdbcOptionsBean, JdbcConnection>  CACHE... 

JDBCClient.createShared(<usual options>, "apiman-" + RandomUUID); <-- we'll manage the mapping ourselves via the cache/map above, so key doesn't matter and we can avoid collision issues

In worst case it would end up creating more pools than needed if people pass in lots of different configuration permutations, but it should avoid ever accidentally getting caught out (e.g. user passes an extra option that means it's no longer compatible with the simple jdbcUrl+username cache key scheme).

Thoughts?

I think if we would use the complete config as cache key then we would´t need a shared client anyway?
Then we could use a normal client (which ensures its created with a unique id).

Only question is what happens with all the clients in the map? There would be no limit of different clients then?
Not sure if this could cause problems?

I think if we would create a unique dsName already here, it would be solved as well:

  public IJdbcClient createStandalone(JdbcOptionsBean config) {
        return new VertxJdbcClientImpl(vertx, config);
    } 

Ah, I did not explained it in a good way.
I thought about a combination.
Maybe its not a good idea just thinking loud :D

1. implement hashCode/equlas
2. use the hashCode as unique dsName

So we would create a shared client which is still unique and vertx would handle it for us.
Then we may do not even need an additional Map<config,clients>