Agroal and Reactive datasources config namespace conflict

[Guillaume Smet]

Hi,

The Agroal extension and the Reactive datasources extensions share the same configuration namespace: `quarkus.datasource`.

That means that you cannot do something like that:

- have an Agroal datasource for ORM

- have a Reactive datasource to connect to another datastore

We had a user trying to do that with no luck.

I'm also pretty sure you cannot have this configuration working:

- one reactive datasource to connect to PostgreSQL

- one reactive datasource to connect to MySQL

Which also looks like a perfectly valid combination.

I think this was a mistake in the first place. Should we fix it or live with it? Obviously, it will break compatibility one way or another.

And if we decide to fix it, what would be your preferred way of doing so?

--

Guillaume

[Thomas SEGISMONT]

Hi,

This wasn't a mistake but a known limitation of the PR that introduced reactive SQL clients.

The solution is to support named datasources in reactive SQL clients.

Regards,

Thomas

--
You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CALt0%2Bo-i%3DoFrK8Q6jRfqPEGrR05Osng1CmwshWGikV3%2Br5Kxvw%40mail.gmail.com.

[Guillaume Smet]

On Mon, Dec 16, 2019 at 2:42 PM Thomas SEGISMONT <tsegi...@gmail.com> wrote:

This wasn't a mistake but a known limitation of the PR that introduced reactive SQL clients.

The solution is to support named datasources in reactive SQL clients.

How does it solve the issue?

You can't have two config root classes sharing the same root.

If we wanted to fully support that, we would need a shared config root and a way to differentiate which extension manages which datasource. That's one of the possibility to solve this particular issue.

The other I can see being different roots.

--

Guillaume

[Thomas SEGISMONT]

Le lun. 16 déc. 2019 à 14:55, Guillaume Smet <guillau...@gmail.com> a écrit :

On Mon, Dec 16, 2019 at 2:42 PM Thomas SEGISMONT <tsegi...@gmail.com> wrote:

This wasn't a mistake but a known limitation of the PR that introduced reactive SQL clients.

The solution is to support named datasources in reactive SQL clients.

How does it solve the issue?

In your first use case, the user can configure the ORM with the default datasource and the Reactive SQL client with a named datasource.

In your second use case, the user can configure the Reactive Pg Client with a named datasource, and the Reactive MySql Client with another named datasource.

 

You can't have two config root classes sharing the same root.

I've heard this before and asked where this limitation is coming from but got no reply.

 

If we wanted to fully support that, we would need a shared config root and a way to differentiate which extension manages which datasource. That's one of the possibility to solve this particular issue.

 

Yes, we've talked about this already.

The other I can see being different roots.

This is what the initial decision tried to avoid.

 

--

Guillaume

[Guillaume Smet]

On Mon, Dec 16, 2019 at 3:20 PM Thomas SEGISMONT <tsegi...@gmail.com> wrote:

In your first use case, the user can configure the ORM with the default datasource and the Reactive SQL client with a named datasource.

In your second use case, the user can configure the Reactive Pg Client with a named datasource, and the Reactive MySql Client with another named datasource.

Well... you miss the important point here: we need some shared config and we need to be able to point to the right extension if you expect that to work.

This requires to fix the underlying configuration issue and to be able to determine which extension should handle the datasource. That's the reason of my email. I'm not entirely sure it will fly if we want some Agroal-specific or reactive-specific configuration. We might be able to make it work but, for sure, config classes will be ugly.

Adding "named datasource" support to the reactive extensions won't solve the issue per se.

You can't have two config root classes sharing the same root.

I've heard this before and asked where this limitation is coming from but got no reply.

Well, to which class will you point the config if two extensions share the same root with most of the properties being the same?

Will your quarkus.datasource.username be for Agroal or one of your reactive extensions? So should we populate the Agroal config class or the Reactive PostgreSQL config class?

Note that we have named datasource support for Agroal so the question is the same for quarkus.datasource.<my-name>.username.

--

Guillaume

 

[Emmanuel Bernard]

I don't fully understand your concern but wouldn't having a technical extension named datasource that publish a build item consumable by agroal and the reactive db be enough?

--

You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CALt0%2Bo-B42KdSuVyBdwmrrCdAJy7fNN24QzNWgm_KS9K17SG5A%40mail.gmail.com.

[Stephane Epardaud]

The plan originally was to abstract datasource config away from agroal and share it between agroal and reactive. Then agroal and reactive would only care about datasources with drivers they support.

--

You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CALt0%2Bo-B42KdSuVyBdwmrrCdAJy7fNN24QzNWgm_KS9K17SG5A%40mail.gmail.com.

--

Stéphane Épardaud

[Guillaume Smet]

On Mon, Dec 16, 2019 at 3:38 PM Emmanuel Bernard <eber...@redhat.com> wrote:

I don't fully understand your concern but wouldn't having a technical extension named datasource that publish a build item consumable by agroal and the reactive db be enough?

My concern is that what we currently have doesn't work, that we had 2 reports lately and that we need to fix it. Nothing more.

Keeping the `datasource` namespace for all can work as long as we have a shared config root (i.e. not the way it is right now with one root per extension) and it will have constraints - see below. The other option is to use different roots, which looks like something you wanted to avoid.

 

Your proposal could work as long as:

- configuration is mostly similar. Dealing with specific configuration might get ugly. Agroal currently has far more configuration knobs than the Reactive datasources. Either we are able to consume the config for the reactive datasources or some configuration knobs will work and some not.

- we add something to differentiate which extension should deal with which datasource: using URL is not a possibility as it's runtime defined. One possibility would be to use the driver and introduce the driver in the reactive extensions.

Again, I'm not saying it can't be fixed, I'm saying we need to fix it and the current situation is not acceptable.

--

Guillaume

[Stephane Epardaud]

Well yeah, we've wanted to fix it for months and still lack the person to do it :(

--

You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CALt0%2Bo_xZuuun8O-Qw3-z-1zFXj-To2%2BNs82VxxT_uw_SfFF1Q%40mail.gmail.com.

--

Stéphane Épardaud

[Guillaume Smet]

Well, let's make a clear plan, create an issue and I'm pretty sure I will be able to find someone.

What should we do for the configuration knobs that have a sense only for Agroal? I see the specific reactive configuration is in its own namespace.

[David Lloyd]

I just want to +1 everything that Guillaume and Stephane have said
here. The common configuration classes need to be extracted to a
shared module (or, if the build time config is also shared, then
shared modules since we'd need a deployment SPI one as well).

As for why named data sources don't fix the problem: it's the
extension loader itself that will fail if two configuration properties
on the class path have the same property name. It is disallowed, and
therefore it is generally not a good idea to do it even if two
extensions with the same property are unlikely to be used together
(sometimes the user's class path isn't always 100% under their
control, and so we should generally be as tolerant as possible).

We have one agreed strategy for when two conflicting extensions
coexist on the class path, and that is to support a configuration-side
flag that disables one or the other, and this means we can't have
conflicting property names. We do not want the user to have to cope
with exclusions or other build system nonsense if we can possibly
avoid doing so.

--
- DML

[David Lloyd]

They don't need a distinct namespace IMO; only the actual conflicting
properties need to be factored out. If necessary we could add a
`...reactive.enable=false` and/or `...agroal.enable=false` kind of
switch I think.

If you're using both reactive and non-reactive in the same project,
pointing to the same database, you're probably going to want to use
the same credentials and other common config too, right?

> To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CALt0%2Bo_eoryEXKdfd_WrNLE-eU7%3DENZrawBQ%3DwAAdg62C0MmVA%40mail.gmail.com.



--
- DML

[Manyanda Chitimbo]

> Well, let's make a clear plan, create an issue and I'm pretty sure I will be able to find someone.

There is this issue https://github.com/quarkusio/quarkus/issues/2751 that @Stephane Epardaud  and @sanne.g...@gmail.com had interesting discussions about reactive/agroal datasource.

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CANghgrRov-9oCaYbKZ-nA%3DYzoEUmsayrCwdGSGKv2MFcun4bpA%40mail.gmail.com.

--

Manyanda Chitimbo.

[Thomas SEGISMONT]

Le lun. 16 déc. 2019 à 16:14, David Lloyd <david...@redhat.com> a écrit :

They don't need a distinct namespace IMO; only the actual conflicting
properties need to be factored out.  If necessary we could add a
`...reactive.enable=false` and/or `...agroal.enable=false` kind of
switch I think.

If you're using both reactive and non-reactive in the same project,
pointing to the same database, you're probably going to want to use
the same credentials and other common config too, right?

Yes. One reported use case was having the Flyway extension for dat migration on startup and the Reactive Pg Client extension for the rest of the app lifetime.

[Stephane Epardaud]

On Mon, 16 Dec 2019 at 16:13, David Lloyd <david...@redhat.com> wrote:

As for why named data sources don't fix the problem: it's the
extension loader itself that will fail if two configuration properties
on the class path have the same property name.  It is disallowed, and
therefore it is generally not a good idea to do it even if two
extensions with the same property are unlikely to be used together

That was done as a temporary workaround because we did not have time to extract config to a common module, in order for the settings to be forward-compatible when we would do the change, on the assumption that the first users would be unlikely to use both blocking and reactive datasources. 

--

Stéphane Épardaud

[Stuart Douglas]

I don't think we can make that assumption. We are trying to unify reactive and imperative to allow the user to just pick what is right for their use case, so it is pretty easy to see scenarios where they want to mix both reactive and blocking datasources, e.g. you might want to code your rarely used admin endpoints up with a simple blocking datasource, but implement the actual business logic in a reactive manner.

Stuart

 

--

Stéphane Épardaud

--
You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAKU9E9t6mKi%3DD54LmhD-iBYe_WJ47xrJXJ8n6kJFK2qqHtW2cA%40mail.gmail.com.

[Andy Guibert]

We do have this issue open for addressing the broken situation: https://github.com/quarkusio/quarkus/issues/2751 which is assigned to me. I've been holding off trying to implement a solution because it sounds like we do not have consensus among the more senior members of the team of how we should solve this.

I've posted my $0.02 on the issue here, which essentially involves separate config roots of `quarkus.datasource.*` for JDBC-based config and `quarkus.vertx.sql-client.*` for Vertx-reactive based config. Thinking about this with my user-hat on (I'm pretty new to the inner-workings of Quarkus still) this seems like the most straightforward solution -- no magic or tricks involved and WYSIWYG. As Thomas mentioned here the most common properties (url/user/pass) do primarily overlap, although formats and property names differ slightly between JDBC vs. Vertx-reactive based config. However, as David pointed out here common property values (e.g. DB username/password) can easily be factored out into separate variables by the user.

On Monday, December 16, 2019 at 9:10:02 AM UTC-6, Guillaume Smet wrote:

Well, let's make a clear plan, create an issue and I'm pretty sure I will be able to find someone.

What should we do for the configuration knobs that have a sense only for Agroal? I see the specific reactive configuration is in its own namespace.

On Mon, Dec 16, 2019 at 4:03 PM Stephane Epardaud <stephane...@gmail.com> wrote:

Well yeah, we've wanted to fix it for months and still lack the person to do it :(

On Mon, 16 Dec 2019 at 15:54, Guillaume Smet <guilla...@gmail.com> wrote:

On Mon, Dec 16, 2019 at 3:38 PM Emmanuel Bernard <eber...@redhat.com> wrote:

I don't fully understand your concern but wouldn't having a technical extension named datasource that publish a build item consumable by agroal and the reactive db be enough?

My concern is that what we currently have doesn't work, that we had 2 reports lately and that we need to fix it. Nothing more.

Keeping the `datasource` namespace for all can work as long as we have a shared config root (i.e. not the way it is right now with one root per extension) and it will have constraints - see below. The other option is to use different roots, which looks like something you wanted to avoid.

 

Your proposal could work as long as:

- configuration is mostly similar. Dealing with specific configuration might get ugly. Agroal currently has far more configuration knobs than the Reactive datasources. Either we are able to consume the config for the reactive datasources or some configuration knobs will work and some not.

- we add something to differentiate which extension should deal with which datasource: using URL is not a possibility as it's runtime defined. One possibility would be to use the driver and introduce the driver in the reactive extensions.

Again, I'm not saying it can't be fixed, I'm saying we need to fix it and the current situation is not acceptable.

--

Guillaume

--
You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.

To unsubscribe from this group and stop receiving emails from it, send an email to quark...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CALt0%2Bo_xZuuun8O-Qw3-z-1zFXj-To2%2BNs82VxxT_uw_SfFF1Q%40mail.gmail.com.

--

Stéphane Épardaud

[Sanne Grinovero]

As Andy, Thomas and Stephane mentioned, we've been discussing this
topic extensively on #2751 and indeed there's no consensus.

I guess I was the primary nay-sayer of having them merged; as
Guillaume I believe it was a mistake to have them share the root .
In my mind the two components are too different to shoehorn into one
set of configurations properties, which would result in hard to
validate configurations and hence bad experience.

I'm also not sold on all the pitfalls people will have by the fact
that the two components are simply not of the same type; we won't be
able to inject either/or depending on say the driver only so things
have to match.

But I understand that conceptually the two things are very much "the
same stuff" and it's compelling to remove the incidental complexity
from the user's minds. I guess I can stand back with my concerns but
please let's invest properly in addressing the various sources of
confusion; not least remember we can't drive the "need a different
extension" after having parsed the configuration / analyzed the usage
by the application, so we need to be prepared to throw various user
friendly errors to guide people out of the mess they will get into.

Let's not forget also that people might want to consume "the same
datasource" from both a reactive component and a non-reactive
component. You'll have to make that work, and this implies the
non-shared configuration properties will need to be mixe-able, and
still validate correctly - as long as both kinds of extensions are
available.

This also imples tuning options will need to be different: for example
even if both support the notion of pool size boundaries, they will
very likely need different sizing. So you can't actually share those
properties even if both have them (in some similar form); if you want
to allow the same "datasource" to be used by both a reactive and a
non-reactive consumer; you also will want to make sure that it's clear
as day that these pools are not shared to avoid confusion.

Frankly it sounds like a recipe for a disaster to me so I'm skeptical
but if this is what you all want, I think Andy could start exploring
the shared-root implementation but with Stephane and Thomas to review
and help with further maintenance please, as all consequential
problems are on you :-P

Thanks,
Sanne

> To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/bd76b42d-794c-4a48-bcf3-44714cba9995%40googlegroups.com.

[Stephane Epardaud]

> Let's not forget also that people might want to consume "the same
> datasource" from both a reactive component and a non-reactive
> component

Not sure what you mean here: they have different drivers and different APIs, so I expect not. The only exception that comes to mind is indeed Flyway, and just because we want to hack it to pretend it works on reactive datasources to avoid declaring a JDBC datasource "just" for it. I hope that's a super special case that we won't replicate.

--

Stéphane Épardaud

[Sanne Grinovero]

On Tue, 17 Dec 2019 at 11:15, Stephane Epardaud
<stephane...@gmail.com> wrote:
>
> > Let's not forget also that people might want to consume "the same
> > datasource" from both a reactive component and a non-reactive
> > component
>
> Not sure what you mean here: they have different drivers and different APIs, so I expect not. The only exception that comes to mind is indeed Flyway, and just because we want to hack it to pretend it works on reactive datasources to avoid declaring a JDBC datasource "just" for it. I hope that's a super special case that we won't replicate.

Sure they have different types, which is what I've been saying all
along. But you want to hide this difference; a consequence is that
people will expect to be able to use these lines as configuration
source for different purposes.

Flyway is a great example; did you expect to make special ad-hoc hacks
to make Flyway work, and nothing else? I'd rather you took Flyway as
an example of the many things people will want to do with their
datasources.

> To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAKU9E9s0pRw6oSG45tCEvw5oe3uRU%3D1VmXCFd2Cx8r1dMLifLA%40mail.gmail.com.

[Stephane Epardaud]

OK, so perhaps the problem is that you consider DataSource to be a type, and thus view a non-JDBC driver to not conform to that type.

Whereas with my user hat on, I consider "quarkus.datasource" to be a "data source" in the English term: a source of data. Can be backed by agroal or whatever.

Perhaps we should have started naming this property "quarkus.this.is.where.my.data.comes.from" to avoid confusion?

In any case, it so happens that so far those sources of data all come from databases, and the fact that we access them using reactive or blocking drivers doesn't hide the facts that:

- they're conceptually the same things: they tell how to connect to a database

- they connect to the same databases, just using a different driver: we don't have a datasource type per JDBC driver type, hopefully

- they share a lot of common configs

- some config is not shared, and in retrospect we should have gotten it out of the root config keys and placed those in "quarkus.datasource.jdbc" and "quarkus.datasource.reactive" but probably that boat has sailed

Now, perhaps we can duplicate the roots into "quarkus.datasource" for JDBC and "quarkus.datasource.rx" for sql-client, but they will still share most config, and it doesn't help our current biggest issue with Flyway because it will require this beauty:

quarkus.datasource.url=jdbc://postgres/table

quarkus.datasource.user=foo

quarkus.datasource.password=bar

quarkus.datasource.rx.url=vertx://postgres/table

quarkus.datasource.rx.user=foo

quarkus.datasource.rx.password=bar

Now, Flyway is currently the only support way in Quarkus to do DB migration, and so I consider it a super special case until/if it supports a reactive data source, in which case we'll be able to remove that hack.

Perhaps it will end up not being special and we'll have to extend this hack to other extensions, but I rather hope not.

Now, I don't have any Lead Hammer, and I won't die on that hill, so I'll accept whatever decision the Hammer doesn't squish, but from a usability standpoint, I think it's hard to convince users that they're different.

[Stephane Epardaud]

On Tue, 17 Dec 2019 at 12:45, Stephane Epardaud <stephane...@gmail.com> wrote:

OK, so perhaps the problem is that you consider ...

Hold on, that came out wrong: I meant to say that "the problem is that we view things from a different angle", not that "the problem is how you view things" :) 

[David Lloyd]

On Tue, Dec 17, 2019 at 5:07 AM Sanne Grinovero <sa...@hibernate.org> wrote:
>
> As Andy, Thomas and Stephane mentioned, we've been discussing this
> topic extensively on #2751 and indeed there's no consensus.
>
> I guess I was the primary nay-sayer of having them merged; as
> Guillaume I believe it was a mistake to have them share the root .
> In my mind the two components are too different to shoehorn into one
> set of configurations properties, which would result in hard to
> validate configurations and hence bad experience.
>

> [...]

>
> Let's not forget also that people might want to consume "the same
> datasource" from both a reactive component and a non-reactive
> component. You'll have to make that work, and this implies the
> non-shared configuration properties will need to be mixe-able, and
> still validate correctly - as long as both kinds of extensions are
> available.

I don't want to diminish your concerns, which I believe are
legitimate, but I will say that the configuration system should handle
this mixability and the consequent validation requirements adequately.

> This also imples tuning options will need to be different: for example
> even if both support the notion of pool size boundaries, they will
> very likely need different sizing. So you can't actually share those
> properties even if both have them (in some similar form); if you want
> to allow the same "datasource" to be used by both a reactive and a
> non-reactive consumer; you also will want to make sure that it's clear
> as day that these pools are not shared to avoid confusion.

Since we're talking about two different pools (two different *kinds*
of pools really), it would make sense to configure them independently,
for example by having a blocking pool size and a reactive pool size
(with supporting documentation to that effect). The two access
methods should also be independently enableable.

However, several configuration properties for data source types are
undoubtedly overlapping in name *and* are semantically unambiguous,
configuring the same concepts (username and password for example). It
should not be a problem to share these properties between the
different access methods, because they apply to the destination, not
the access method.

So, I agree with all of you. :-)

--
- DML

[Emmanuel Bernard]

I actually found David's proposal reasonable unless someone complains

quarkus.datasource.url=jdbc://postgres/table
quarkus.datasource.user=foo
quarkus.datasource.password=bar
quarkus.agroal.on-error=shut-up

[David Lloyd]

You seem so surprised! ;-)

--
You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/C6E0423A-C6F1-4564-BAE5-18D4B2E616F8%40redhat.com.

--

- DML

[Andy Guibert]

Would this be the complete configuration for a user that wants to use a JDBC DataSource and a Vertx reactive client? If I understand correctly, the Vertx module would just swap out `jdbc://` for the vertx protocol in the URL and re-use all of the other properties?

[Emmanuel Bernard]

I am indeed :D

[Emmanuel Bernard]

Good question, I don't think sharing the high level values work. I was after sharing the high level structure.

So for flyway it would be

deps:
- reactive-postgresql-extension
- jdbc-postgresql-extension

#reactive bit
quarkus.datasource.forapp.url=reactive:postgresql://postgres/table
quarkus.datasource.forapp.user=foo
quarkus.datasource.forapp.password=bar

#blocking bit
quarkus.datasource.forflyway.url=jdbc://postgres/table
quarkus.datasource.forflyway.driver=org.postgresql.Driver #should be quarkus.agroal.forflyway.driver?
quarkus.datasource.forflyway.user=foo
quarkus.datasource.forflyway.password=bar
quarkus.agroal.app.on-error=shut-up

I'm not sure if people were after one datasource for both reactive and imperative in the same app from a single declaration or not. But again that's a bit of a special case Flyway not being reative friendly but wanting to share the same config as it's seen as a single unit of work (schema + SQL statements).

Emmanuel

--
You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/88597cbe-a3b1-4b0e-98c9-14a0c20d172e%40googlegroups.com.

[Andy Guibert]

So in that configuration example, what is the value of `quarkus.agroal.app.on-error=shut-up`?  If people are going to have to configure 2 separate blocks for reactive and imperative data sources anyway, why not let them have separate config roots where each has full control over the listed/documented config? This would avoid awkward situations in doc where we have to say "property foo applies to reactive but not imperative" or "property X here corresponds to property Y on the Vertx-sql-client API".

--

Andy

You received this message because you are subscribed to a topic in the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/quarkus-dev/3r0lquVsUsc/unsubscribe.
To unsubscribe from this group and all its topics, send an email to quarkus-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/5C6DD610-CF61-423B-819B-779B7BE62004%40redhat.com.

[Stuart Douglas]

On Thu, 19 Dec 2019 at 03:10, Andy Guibert <andy.g...@gmail.com> wrote:

So in that configuration example, what is the value of `quarkus.agroal.app.on-error=shut-up`?  If people are going to have to configure 2 separate blocks for reactive and imperative data sources anyway, why not let them have separate config roots where each has full control over the listed/documented config? This would avoid awkward situations in doc where we have to say "property foo applies to reactive but not imperative" or "property X here corresponds to property Y on the Vertx-sql-client API".

+1

I struggle to see how this sharing of config keys creates anything other than confusion, especially given that we have some common use cases where the user will want both a reactive and blocking datasource (namely flyway + reactive).

None of our other config works like this, everything else has specific config that is owned by an extension. If the config cannot be shared by reactive and blocking databases, and each extension needs to implement its own config roots anyway for non standard properties I really struggle to see how this could be considered to be more user friendly than just having separate config for reactive and blocking. 

If we do want to keep these shared keys then I think we should do something like the following:

- The shared keys are for defining 'simple' data sources that do not require any obscure properties, only common values that can be supported for all types are defined in this namespace. This config object will be moved to a shared extension.

- Agroal and Reactive introduce their own config, that also allows you to set the common properties as well, so if you want to define a blocking datasource you can just do it directly using quarkus.agroal properties.

- Datasources defined using the common properties will create both reactive and blocking datasources if both extensions are installed, and there are any consumers of the given type in the app. Usage can either be direct injection or another extension such as flyway or hibernate that wants to use it. 

This has the following advantages:

- Most apps will only use the common datasource config, so will not require any migration, and the config is simple and what you would expect

- Flyway will 'just work' with reactive, same with mixing JPA + reactive in the same app, only a single datasource needs to be defined.

- If you want to use advanced agroal config you can do it all in the agroal extension without mixing config roots

- If you explicitly want a reactive or agroal DS you can define it using that extensions config root and make it clear what you are after

I think this gives a good mix of having things 'just work' so the user does not need to duplicate config, while also allowing for explicit config that lets them control exactly what is going on. It is also easy to document that the quarkus.datasource properties are shared and can be used by all data providers, while the extension specific ones only apply to a given extension.

Stuart

 

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAHiR6Z_BmSg_Vcjpp4y-b7qph04iJtt5_YGDLBpHBvf8JECWnQ%40mail.gmail.com.

[Emmanuel Bernard]

Yes I am warming up to the idea. The simplification I was hoping is not materializing.

[Stephane Epardaud]

What idea? Can you guys show us config examples?

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/A012B47C-12A7-42F6-ACCC-2D753FDEB249%40redhat.com.

--

Stéphane Épardaud

[Emmanuel Bernard]

Taking my previous example

deps:
- reactive-postgresql-extension
- jdbc-postgresql-extension

#reactive bit
quarkus.reactive-datasource.url=reactive:postgresql://postgres/table
quarkus.reactive-datasource.user=foo
quarkus.reactive-datasource.password=bar

#blocking bit
quarkus.datasource.url=jdbc://postgres/table
quarkus.datasource.driver=org.postgresql.Driver #should be quarkus.agroal.forflyway.driver?
quarkus.datasource.user=foo
quarkus.datasource.password=bar
quarkus.datasource.on-error=shut-up

So we clearly say that blocking and non blocking datasources are different beasts and that extensions will know whether they want to use the blocking or non blocking one. Therefore these are two different namespace not only in config name but also in data source names. If you want a blocking and a non blocking access to the same database, you will dupe the URL and username / password.

Flyway for reactive again can be treated as a special workaround case.

[Stephane Epardaud]

OK. I fail to see how that is better than:

#reactive datasource a
quarkus.datasource.a.url=reactive:postgresql://postgres/table
quarkus.datasource.a.user=foo
quarkus.datasource.a.password=bar

#blocking datasource b
quarkus.datasource.b.url=jdbc://postgres/table
quarkus.datasource.b.user=foo
quarkus.datasource.b.password=bar
quarkus.datasource.b.jdbc.driver=org.postgresql.Driver #should be quarkus.agroal.forflyway.driver?
quarkus.datasource.b.agroal.on-error=shut-up

I've added names because this is how we define more than one datasource. If "driver" and "on-error" are not shared between jdbc/agroal/sql-client they move to their own namespace, but perhaps they can be used by both.

[Andy Guibert]

This is a little better, because you've modified the config example a bit to have JDBC-only properties under a separate "quarkus.datasource.{name}.jdbc" subdomain:

quarkus.datasource.b.jdbc.driver=org.postgresql.Driver

And then vertx-only config might have something like:

quarkus.datasource.a.vertx-reactive.idleTimeout=30

However, I also fail to see how this is much better than separate config roots? I know that you think of "datasource" in this context as "source of my data", but not all users will make that association. In my mind, the term "datasource" has been claimed by JDBC with java.sql.DataSource, and since Vertx-sql-client does not use JDBC or java.sql.DataSource, it seems improper to call this configuration a "datasource".

To re-post my proposed configuration from the github issue, this is what I had in mind:

#reactive datasource a
quarkus.vertx.sql-client.a.url=reactive:postgresql://postgres/table
quarkus.vertx.sql-client.a.user=foo
quarkus.vertx.sql-client.a.password=bar

#blocking datasource b
quarkus.datasource.b.url=jdbc://postgres/table
quarkus.datasource.b.user=foo
quarkus.datasource.b.password=

bar
quarkus.datasource.b.driver=org.postgresql.Driver

I see the following benefits with this approach:

 - Vertx-sql-client has complete control over configuration, and documentation will be grouped together with other Vertx configuration

 - Property names are shorter: JDBC-only configuration properties don't need to go under a ".jdbc" subdomain, likewise for a Vertx-sql-client subdomain

 - Does not attempt to overload the term "datasource" with meaning something other than java.sql.DataSource

 - Having a configuration root _after_ a named resource seems a bit odd. Inevitably people will try things like: quarkus.datasource.jdbc.b.driver=.... If we don't already have this pattern somewhere, it would be good to avoid. The <config-root>.<optional-name>.<property-name> pattern is nice

--

Andy

[Johnny T]

Hi,

there is one thing I don’t get and maybe there is someone that could explain it:

The reactive-pg-client does not reinvent agroal and its Microprofile Standard DataSource-Type. It’s more like an „wrapper“ or „adapter“, that put a different „view“ on the Standard-DataSource-Type it’s based upon. Even if it is also an source of data language wise, it shouldn’t be named the same way IMO as the thing it is based on.

My question: Would it be possible to rewrite some parts of the reactive-pg-client to be more like a wrapper/adapter on top of the Standard-DataSource? An extension, that can be selected like flyway, that can be used for whatever datasource you like?

It could provide a “ReactiveDataSource“ to communicate that more clearly.

But i guess, this wouldn’t be easy since it communicates in the opposite direction and depends on PostgreSQL specific commands?

Johnny.

[Andy Guibert]

> The reactive-pg-client does not reinvent agroal and its Microprofile Standard DataSource-Type.

I'm not sure what you mean by this? The Vertx reactive PG client we are referring to is based off of the io.vertx.sqlclient.SqlClient class which does _not_ use Agroal or java.sql.DataSource. Not sure what you mean by "MicroProfile Standard DataSource" -- there is no such thing. Perhaps you are confusing the Vertx Reactive SQL Client with the Vertx JDBC Client here?

https://github.com/vert-x3/vertx-jdbc-client/blob/master/src/main/java/io/vertx/ext/jdbc/JDBCClient.java#L90

> Would it be possible to rewrite some parts of the reactive-pg-client to be more like a wrapper/adapter on top of the Standard-DataSource?

The Vertx JDBC client has done this (see above link), but unfortunately it is not possible for the reactive SQL client because `java.sql.DataSource#getConnection` must return a `java.sql.Connection` object, neither of which have any APIs that have the event-based method signatures such as `SqlClient#query(String, Handler<AsyncResult<RowSet<Row>>>)`

--

Andy

[Emmanuel Bernard]

I don't like the ref to vertx in the namespace. I hope it won't happen but I'd rather have a generic reactive-datasource definition here just like we have a generic datasource usage for agroal. So that other techs can reuse that information to boot itself.

--

You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/34d3c2ce-6139-4702-ab76-9c4db3dee746%40googlegroups.com.

[Stephane Epardaud]

OK, so if agroal/jdbc/datasource people would get confused with anything called a data source which isn't java.sql.DataSource, how about we don't confuse the rest and try:

#reactive datasource a

quarkus.database.a.url=reactive:postgresql://postgres/table

quarkus.database.a.user=foo

quarkus.database.a.password=bar

#blocking datasource b

quarkus.datasource.b.url=jdbc://postgres/table

quarkus.datasource.b.user=foo

quarkus.datasource.b.password=bar

quarkus.datasource.b.driver=org.postgresql.Driver

We make two prefixes: one quarkus.database which works for all kinds of data sources, and the existing quarkus.datasource which is only for jdbc. We only check that they can't have the same names (or use both the default name).

People who want to use quarkus.datasource alone have their project unchanged. People who want to mix are encouraged to use the quarkus.database namespace for both sources.

[Sanne Grinovero]

On Tue, 17 Dec 2019 at 11:45, Stephane Epardaud
<stephane...@gmail.com> wrote:
>
> OK, so perhaps the problem is that you consider DataSource to be a type, and thus view a non-JDBC driver to not conform to that type.
>
> Whereas with my user hat on, I consider "quarkus.datasource" to be a "data source" in the English term: a source of data. Can be backed by agroal or whatever.

Let me answer from here first.

Sure I get that. As I said on this same thread, they are conceptually
"the same stuff" for end users; also said it on the issue multiple
times, so fear not I understand that angle:
- https://github.com/quarkusio/quarkus/issues/2751#issuecomment-557508232

But there's a load of small problems stemming from that direction
which I'm not comfortable with; the *type* being different for
injection points being a strong point but not the only one.
Other points are tuning differences and the general confusion; myself
and many others here have made examples of possible confusing points.

To summarize my position is that I sympatise with the idea, but it
gets so tricky that it's hardly worth it?

I do like Stuart's suggestion to fully separate the properties in 3
block: shared, agroal, reactive, and the subsequent suggestions look
promising in terms of configuration properties.

>
> Perhaps we should have started naming this property "quarkus.this.is.where.my.data.comes.from" to avoid confusion?
>
> In any case, it so happens that so far those sources of data all come from databases, and the fact that we access them using reactive or blocking drivers doesn't hide the facts that:
>
> - they're conceptually the same things: they tell how to connect to a database
> - they connect to the same databases, just using a different driver: we don't have a datasource type per JDBC driver type, hopefully
> - they share a lot of common configs
> - some config is not shared, and in retrospect we should have gotten it out of the root config keys and placed those in "quarkus.datasource.jdbc" and "quarkus.datasource.reactive" but probably that boat has sailed
>
> Now, perhaps we can duplicate the roots into "quarkus.datasource" for JDBC and "quarkus.datasource.rx" for sql-client, but they will still share most config, and it doesn't help our current biggest issue with Flyway because it will require this beauty:
>
> quarkus.datasource.url=jdbc://postgres/table
> quarkus.datasource.user=foo
> quarkus.datasource.password=bar
>
> quarkus.datasource.rx.url=vertx://postgres/table
> quarkus.datasource.rx.user=foo
> quarkus.datasource.rx.password=bar
>
> Now, Flyway is currently the only support way in Quarkus to do DB migration, and so I consider it a super special case until/if it supports a reactive data source, in which case we'll be able to remove that hack.
>
> Perhaps it will end up not being special and we'll have to extend this hack to other extensions, but I rather hope not.
> Now, I don't have any Lead Hammer, and I won't die on that hill, so I'll accept whatever decision the Hammer doesn't squish, but from a usability standpoint, I think it's hard to convince users that they're different.
>

> --
> You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAKU9E9tXU_ige6RGTh_648%2B627g9B3uKGLS9Nw3G%2BYobFvKf6Q%40mail.gmail.com.

[Stephane Epardaud]

> I do like Stuart's suggestion to fully separate the properties in 3
block: shared, agroal, reactive, and the subsequent suggestions look
promising in terms of configuration properties.

I'm not sure I've seen an example of that?

--

Stéphane Épardaud

[Sanne Grinovero]

I like this better, but I guess that doesn't surprise you now ;-)

Still some issues pending: say you have just:

quarkus.database.a.url=reactive:postgresql://postgres/table
quarkus.database.a.user=foo
quarkus.database.a.password=bar

as the user's intent is to use Reactive primarily; is it ok to you all
that the injection point type of the "Datasource" will changes type
when the url is reconfigured to a non-reactive url?

N.B. I've been annoying you about the actual *type* being different,
as dependency injection being resolved at build time, it implies that
this specific property would not be able to change from reactive to
non-reactive after build; that's a new class of configuration
properties, and would require a new kind of runtime validation.

N.B./2 which extensions do I need on classpath?
Documentation will need to reiterate all over that the dependencies
needed in the project are going to be different depending on such
properties. I don't like that as it's going to be hard to provide easy
to understand error messages but I suppose you might be able to
mitigate this in various ways - just please don't ignore this
additional hurdle.

Tangentially, I can see some value in wanting to use Flyway during
bootstrap, without having a full blown Agroal datasource running all
the time. Maybe we could either :
- contribute to Flyway to have it consume reactive sql "datasources"
as an alternative?
- have an adaptor in Quarkus?
- or simply have a way to shut down the Agrol datasource after such
bootstrap processes have completed.

Thanks,
Sanne

>
> --
> You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAKU9E9tC2D_Kj5QCi_0MXXEG8BU51LXfD8MCka66MbVudNZ9Rw%40mail.gmail.com.

[Emmanuel Bernard]

That's the first example I got but my pov is that it failed to solve the problem because these are too deeply different things, see my other messag ein a minute or two.

[Stephane Epardaud]

On Fri, 20 Dec 2019 at 11:52, Sanne Grinovero <sa...@hibernate.org> wrote:

as the user's intent is to use Reactive primarily; is it ok to you all
that the injection point type of the "Datasource" will changes type
when the url is reconfigured to a non-reactive url?

It's not exactly that it changes type, it's more that EntityManager injections stop working and SqlClient start working when you change it, but it feels similar in effect, I agree.

Didn't think about that. I guess we don't have an equivalent when changing jdbc drivers.

I would say that probably it would bother me, but that change also implies replacing a quarkus-hibernate-orm dependency for a quarkus-sql-client (forgot its exact name) one, so

it's not like it'd be a config change alone that would lead to this. In most cases your IDE would throw lots of red at you for just using those types when you switch dependency,

even before you do any config change. So, perhaps it's an issue that isn't as big as it may appear?

 

N.B. I've been annoying you about the actual *type* being different,
as dependency injection being resolved at build time, it implies that
this specific property would not be able to change from reactive to
non-reactive after build; that's a new class of configuration
properties, and would require a new kind of runtime validation.

Yeah, it totally would not be able to change at run time, but that's also due to them having different module dependencies. I forgot if we can change jdbc driver at run time, can we?

 

N.B./2 which extensions do I need on classpath?
Documentation will need to reiterate all over that the dependencies
needed in the project are going to be different depending on such
properties. I don't like that as it's going to be hard to provide easy
to understand error messages but I suppose you might be able to
mitigate this in various ways - just please don't ignore this
additional hurdle.

I don't see this as different to the required jdbc extension on the side.

 

[Emmanuel Bernard]

The NB points of Sanne are really nailing down the points I was trying
to make.
Since literally the app code will be different depending of the
datasource access type (imp or reactive), I would like to better
understand the benefit of a property name alignment (or sharing).
Imagine say Quartz with DB serialization of the job state. It's using
blocking today, so you have to say by documentation or by failing the
build that sorry your DB type is wrong and you cannot use reactive here.
That's why I relaxed a lot into accepting that blocking and non blocking
DB access are really two different things and it's fine to expose that
to the user. Principle of least surprised.

The first NB in an eye opener because we do expect people to be able to
change the URL at runtime but not "that much".

[Jason Greene]

On Dec 20, 2019, at 5:07 AM, Stephane Epardaud <stephane...@gmail.com> wrote:

On Fri, 20 Dec 2019 at 11:52, Sanne Grinovero <sa...@hibernate.org> wrote:

as the user's intent is to use Reactive primarily; is it ok to you all
that the injection point type of the "Datasource" will changes type
when the url is reconfigured to a non-reactive url?

It's not exactly that it changes type, it's more that EntityManager injections stop working and SqlClient start working when you change it, but it feels similar in effect, I agree.

Didn't think about that. I guess we don't have an equivalent when changing jdbc drivers.

Isn’t it effectively the same for jdbc? Once you perform a build with a particular driver you can’t have a URL refer to a different one. IMO it’s fine to restrict the legal runtime set based on build time (e.g.  you can change the host but not the driver type),  

The way I would think about this is do the reactive and jdbc drivers have mostly the same properties in total. If they do then I think its a strong reason to avoid a major change. So basically, assuming that, I am agreeing with you We want a good solution, but we also don’t want to overreact.

-Jason

[David Lloyd]

As a user, this is how _I_ would want the configuration to look:

# Every database type has an address

quarkus.datasource.my-db.address = database.foo.com:1234 # run time

# And supports SSL, I'd hope

quarkus.datasource.my-db.ssl = true # run time, I guess

# I don't want to give a driver class name, yuck

quarkus.datasource.my-db.kind = postgresql # build time

quarkus.datasource.my-db.user = foo # run time

quarkus.datasource.my-db.password = bar # run time

# I want JDBC

quarkus.datasource.my-db.jdbc = true # build time

quarkus.datasource.my-db.jdbc.pool-size = 5 # run time

# I want reactive too

quarkus.datasource.my-db.reactive = true # build time

quarkus.datasource.my-db.reactive.pool-size = 15 # run time

# I need to configure some posgres-specific stuff

quarkus.datasource.my-db.postgresql.database = prod # run time

%dev.quarkus.datasource.my-db.postgresql.database = dev

 # just an example of a possible DB-specific property

quarkus.datasource.my-db.postgresql.prepare-threshold = 10

Then I'd want to only have to bundle:

* quarkus-agroal-postresql (or something like that)

* quarkus-reactive-pg-client

and all of the other needed stuff comes in transitively.

Note the lack of ugly/gross JDBC URLs.  Note the points where properties are shared and not shared.  Note that the DB address/SSL config can be run time with the driver kind can be build time, avoiding sticky inconsistency around URL changes.

I would expect injection to Just Work based on the injected type.  If it didn't, as a user I would be disappointed for sure.

I will add that the config system can support this example config today, so don't worry about that aspect.

As a user, will I be disappointed?  Why?

--
You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/AAC43A9C-8163-46C7-9435-E33F328A5EBE%40redhat.com.

--

- DML

[Jason Greene]

So I completely agree with you that JDBC urls suck. There is just too much variation in syntax. On the other hand they are well known/understood, so its probably worth still supporting them even if we have an alternative form.

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CANghgrTsbqN65MCjKSn0A3O1AMEs9OqVTFp_Mni%3DKjPyq18yEw%40mail.gmail.com.

[Stephane Epardaud]

Yeah, getting rid of JDBC URLs and driver class names has my vote.

I like your idea David, but the number of discussions this week kinda tells me we need to take more time to discuss things and explore options before rushing to a decision. I know the issue has been open for almost a year, and we're late actively participating in the discussion, but I feel we've made a lot of progress in trials and requirements and I'm sure this will mature in our heads over the holidays and early January we'll come to an agreement, or at least a decision.

I do like your proposal very much, though.

--

Stéphane Épardaud

[Guillaume Smet]

On Fri, Dec 20, 2019 at 3:28 PM Stephane Epardaud <stephane...@gmail.com> wrote:

Yeah, getting rid of JDBC URLs and driver class names has my vote.

We are working with Anamarija to have the infrastructure ready to allow that in the future.

But I don't really see how it is related to the issue at hand.

Personally, as a user I would highly prefer to have one namespace fully well defined for a JDBC datasource and one namespace each specific type of datasource.

My configuration is consistent and I have the full list of parameters handy in the documentation.

With what David suggests, the user will see the reactive property, "oh that's cool, let's set it to true", then "oh my, why is my Hibernate ORM setup all borked?".

--

Guillaume

[Sanne Grinovero]

I think David's proposal is very reasonable, and I welcome that e.g.
the pool tuning properties are separated.

I love that I can have a datasource X both reactive *and* not
reactive, and reconfiguration of this aspect is explicit enough that I
don't expect confusion.

We'll need to assume "quarkus.datasource.my-db.jdbc=true" is the
default for backwards compatibility, but it's easy enough to set to
false, or have it controlled by the presence of the needed
dependencies; pretty much everything else except the address should be
optional.

Hopefully no other objections?

[Zihao Yu]

I agree with Guillaume that JDBC datasource and Reactive datasources should have their own namespace.

JDBC datasource and Reactive datasources are totally different technologies. Putting them together can only make things more complicated.

[Guillaume Smet]

On Fri, Dec 20, 2019 at 7:56 PM Sanne Grinovero <sa...@hibernate.org> wrote:

We'll need to assume "quarkus.datasource.my-db.jdbc=true" is the
default for backwards compatibility, but it's easy enough to set to
false, or have it controlled by the presence of the needed
dependencies; pretty much everything else except the address should be
optional.

Hopefully no other objections?

Well, if you ignore mine, probably not ;).

I think it will bite us because tuning the datasource and setting reactive to true has a good chance to break your application. That's definitely not very user friendly.

-- 

Guillaume 

[Stuart Douglas]

I think that both 'reactive' and 'jdbc' should default to true, although only actually take effect if the relevant extension is installed. Even if you set reactive to true JDBC would still work, and worst case you end up with a reactive pool that is never used. Basically these settings would only be for the case that you had both extensions installed and you explicitly wanted a datasource to be reactive only or jdbc only.

Stuart

 

-- 

Guillaume 

--
You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CALt0%2Bo-_hQ59i4ivQ%3D6CSkUHe29V8PB9dmnrABjSUAT97HZM4Q%40mail.gmail.com.

[Sanne Grinovero]

On Sat, 21 Dec 2019 at 18:59, Guillaume Smet <guillau...@gmail.com> wrote:
>
> On Fri, Dec 20, 2019 at 7:56 PM Sanne Grinovero <sa...@hibernate.org> wrote:
>>
>> We'll need to assume "quarkus.datasource.my-db.jdbc=true" is the
>> default for backwards compatibility, but it's easy enough to set to
>> false, or have it controlled by the presence of the needed
>> dependencies; pretty much everything else except the address should be
>> optional.
>>
>> Hopefully no other objections?
>
>
> Well, if you ignore mine, probably not ;)
>

> I think it will bite us because tuning the datasource and setting reactive to true has a good chance to break your application. That's definitely not very user friendly.

Not meaning to ignore your original objection, but I had the
impression we started from the same mind set;

yet I think the latest proposals do a fair job of separating
explicitly all properties which would possibly have been confusing;
even the pool sizes are separated allowing for different defaults
blocking/reactive, and it's crafted in such a way that we should still
be able to automate some boring stuff, such as picking the right jdbc
driver.

Also, type of injection points should not longer change automagically
- unless a very explicit know is flipped which turns off the whole
starting of the reactive|blocking component.

In terms of usability, Stephane, Thomas et al always had good points;
I wasn't convinced that it was worth it considering all the
possibilities of confusing side-effects were strong in the initial
proposals; but I believe that all my counter-arguments and pain points
I raised have been taken care of; I think the latest proposals (see
David's last in particular) might persuade you too - please have
another look?

Thank,
Sanne

[Sanne Grinovero]

On Sun, 22 Dec 2019 at 01:50, Stuart Douglas <sdou...@redhat.com> wrote:
>
>
>
> On Sun, 22 Dec 2019 at 05:59, Guillaume Smet <guillau...@gmail.com> wrote:
>>
>> On Fri, Dec 20, 2019 at 7:56 PM Sanne Grinovero <sa...@hibernate.org> wrote:
>>>
>>> We'll need to assume "quarkus.datasource.my-db.jdbc=true" is the
>>> default for backwards compatibility, but it's easy enough to set to
>>> false, or have it controlled by the presence of the needed
>>> dependencies; pretty much everything else except the address should be
>>> optional.
>>>
>>> Hopefully no other objections?
>>
>>
>> Well, if you ignore mine, probably not ;).
>>
>> I think it will bite us because tuning the datasource and setting reactive to true has a good chance to break your application. That's definitely not very user friendly.

P.s. I don't see how it breaks, I believe you didn't catc up on the
latest evolutions in this thread: unless you removed the non-reactive
extensions, you'd have both types of datasources: dependent frameworks
(say ORM) keep working using the non-reactive sources as before, and
injection points are still functional too - as they are different
types you can have both.

> I think that both 'reactive' and 'jdbc' should default to true, although only actually take effect if the relevant extension is installed.

+1

> Even if you set reactive to true JDBC would still work, and worst case you end up with a reactive pool that is never used. Basically these settings would only be for the case that you had both extensions installed and you explicitly wanted a datasource to be reactive only or jdbc only.

+1

-- Sanne

[Andy Guibert]

I like David's proposal and have no objections to it, assuming we have an answer to this question:

Currently JDBC drivers have dozens of vendor-specific properties that can be specified in the JDBC URL. This is nice because Quarkus does not need to know anything about the custom properties or how they need to fit into the URL. The user simply consults the JDBC driver vendor's documentation. If we change config to the "address based" approach, will it still be possible for users to specify the vendor-specific JDBC driver properties, without Quarkus needing to keep track of and explicitly document each vendor-specific property?

Looking at our current documentation I don't think this is possible. So if we go down the address-based config path, it should be possible for a user to specify config like:

quarus.datasource."datasource-name"."vendor-specific-property"

Which Quarkus could pass along to java.sql.DriverManager#getConnection(String url, Properties info)

Also, I suggested something similar to David's proposal [1] and Stephane was in favor of the URL property at the time, so we should make sure Stephane is OK with David's proposal.

[1]: https://github.com/quarkusio/quarkus/issues/2751#issuecomment-555597232

--

Andy

>> To unsubscribe from this group and stop receiving emails from it, send an email to quark...@googlegroups.com.

[Emmanuel Bernard]

People feel it is ease of use compliant? I have to admit, my initial reaction was that it was complicated but I held off initial judgement. And it's getting better on me.

First, aside from the fairly specific Flyway + reactive DB access where sharing is making things easier, I do think more and more that reactive and JDBC datasources are different and significantly alter how one writes an app. So not sharing the definition is ok for me. Of course if we share and see no drawback, even better.

With that said, couple of questions / remarks:

• I see you set specific postgresql settings. Will I be able to add more and specifically more databases? Should these extra definitions live in the shared "datasource config" technical extension? Or can they be split?
• we most likely need to allow raw JDBC URLs as well. There are many cases where passing a property via the URL is a lifesaver in case the upper framework does not expose some obscure feature of the database.
• I see you used some property as both leaf and node (JDBC), how does that look in YAML?

Emmanuel

PS we would need an old datasource setting to new datasource setting backward compatibility mode.

[David Lloyd]

On Fri, Jan 3, 2020 at 6:33 AM Emmanuel Bernard <eber...@redhat.com> wrote:

People feel it is ease of use compliant? I have to admit, my initial reaction was that it was complicated but I held off initial judgement. And it's getting better on me.

First, aside from the fairly specific Flyway + reactive DB access where sharing is making things easier, I do think more and more that reactive and JDBC datasources are different and significantly alter how one writes an app. So not sharing the definition is ok for me. Of course if we share and see no drawback, even better.

With that said, couple of questions / remarks:

• I see you set specific postgresql settings. Will I be able to add more and specifically more databases?

Yes. 

• Should these extra definitions live in the shared "datasource config" technical extension? Or can they be split?

They should be split.  Each property would only live within its defining extension: generalized datasource support, generalized blocking and/or reactive support, specific driver support. 

• we most likely need to allow raw JDBC URLs as well. There are many cases where passing a property via the URL is a lifesaver in case the upper framework does not expose some obscure feature of the database.

This is something of a recurrent topic, isn't it?  And we vacillate quite a lot about our philosophy here.  I'm still opposed to configuration back doors.  I think it's better to add properties as needed to the per-driver extension.  Maybe even add them all up front.  But I'm not immovable on that.

• I see you used some property as both leaf and node (JDBC), how does that look in YAML?

Ideally it would look like this (one example property):

quarkus:

    datasource:

        my-db:

            jdbc:

                pool-size: 5

with the main property defaulting to `true`.  However this is contingent on having some other required build time property exist within that name space or else having some other special mechanism.  If that doesn't work out, then users will have to specify it explicitly:

quarkus:

    datasource:

        my-db:

            jdbc:

                ~: true

                pool-size: 5

as per previous discussions.

--

- DML

[Emmanuel Bernard]

On 3 Jan 2020, at 13:57, David Lloyd wrote:

> On Fri, Jan 3, 2020 at 6:33 AM Emmanuel Bernard <eber...@redhat.com>
> wrote:
>
>> People feel it is ease of use compliant? I have to admit, my initial
>> reaction was that it was complicated but I held off initial
>> judgement. And
>> it's getting better on me.
>>
>> First, aside from the fairly specific Flyway + reactive DB access
>> where
>> sharing is making things easier, I do think more and more that
>> reactive and
>> JDBC datasources are different and significantly alter how one writes
>> an
>> app. So not sharing the definition is ok for me. Of course if we
>> share and
>> see no drawback, even better.
>>
>> With that said, couple of questions / remarks:
>>

>> - I see you set specific postgresql settings. Will I be able to

>> add
>> more and specifically more databases?
>>
>> Yes.
>
>>

>> - Should these extra definitions live in the shared "datasource

>> config" technical extension? Or can they be split?
>>
>> They should be split. Each property would only live within its
>> defining
> extension: generalized datasource support, generalized blocking and/or
> reactive support, specific driver support.
>
>>

>> - we most likely need to allow raw JDBC URLs as well. There are

>> many
>> cases where passing a property via the URL is a lifesaver in case
>> the upper
>> framework does not expose some obscure feature of the database.
>>
>> This is something of a recurrent topic, isn't it? And we vacillate
>> quite
> a lot about our philosophy here. I'm still opposed to configuration
> back
> doors. I think it's better to add properties as needed to the
> per-driver
> extension. Maybe even add them all up front. But I'm not immovable
> on
> that.

My experience with Hibernate is that while the framework offers a metric
ton of passthrough properties, the URL property passing end up being
useful in this one out of 100 cases where things don't work. So for
Quarkus the ratio will be less favorable.

>
>>
>> - I see you used some property as both leaf and node (JDBC), how

[Sanne Grinovero]

On Fri, 3 Jan 2020 at 12:58, David Lloyd <david...@redhat.com> wrote:
>
> On Fri, Jan 3, 2020 at 6:33 AM Emmanuel Bernard <eber...@redhat.com> wrote:
>>
>> People feel it is ease of use compliant? I have to admit, my initial reaction was that it was complicated but I held off initial judgement. And it's getting better on me.
>>
>> First, aside from the fairly specific Flyway + reactive DB access where sharing is making things easier, I do think more and more that reactive and JDBC datasources are different and significantly alter how one writes an app. So not sharing the definition is ok for me. Of course if we share and see no drawback, even better.
>>
>> With that said, couple of questions / remarks:
>>
>> I see you set specific postgresql settings. Will I be able to add more and specifically more databases?
>
> Yes.
>>
>> Should these extra definitions live in the shared "datasource config" technical extension? Or can they be split?
>
> They should be split. Each property would only live within its defining extension: generalized datasource support, generalized blocking and/or reactive support, specific driver support.
>>
>> we most likely need to allow raw JDBC URLs as well. There are many cases where passing a property via the URL is a lifesaver in case the upper framework does not expose some obscure feature of the database.
>
> This is something of a recurrent topic, isn't it? And we vacillate quite a lot about our philosophy here. I'm still opposed to configuration back doors. I think it's better to add properties as needed to the per-driver extension. Maybe even add them all up front. But I'm not immovable on that.

I'm +1 to not allow "configuration backdoors", we should at least try
to stand our ground there.
Yet having the traditional JDBC URL allows a nice copy/paste
experience from documentation of databases, and other tools, so
there's at least some merit in allowing it - perhaps just veto any
property?

Another reason to separate at least the JDBC properties into ad-hoc
properties is that we'd be able to override the defaults from the JDBC
driver - in some cases they really should: not going to nit-pick now
on specific odd choices, let's keep it at the fact that having this
capability would be nice.

BTW - not sure who said this but it's worth clarifying - it's not
strictly true that all JDBC drivers connect to "some hostname"; some
of the newer storage toys are more PaaS oriented and as such use
different forms of identification encoded in the URL; but that's
simple to support of course, we'd just have different specific
properties for such weird ones :)
Just keep it in mind as the set of properties shared across all
databases is getting smaller.

[Sanne Grinovero]

Sorry but I was unable to fully parse this sentence :) could you
clarify what you mean please?

Thanks

[David Lloyd]

On Fri, Jan 3, 2020 at 12:02 PM Sanne Grinovero <sa...@hibernate.org> wrote:

On Fri, 3 Jan 2020 at 12:58, David Lloyd <david...@redhat.com> wrote:
>
> On Fri, Jan 3, 2020 at 6:33 AM Emmanuel Bernard <eber...@redhat.com> wrote:
>>
>> People feel it is ease of use compliant? I have to admit, my initial reaction was that it was complicated but I held off initial judgement. And it's getting better on me.
>>
>> First, aside from the fairly specific Flyway + reactive DB access where sharing is making things easier, I do think more and more that reactive and JDBC datasources are different and significantly alter how one writes an app. So not sharing the definition is ok for me. Of course if we share and see no drawback, even better.
>>
>> With that said, couple of questions / remarks:
>>
>> I see you set specific postgresql settings. Will I be able to add more and specifically more databases?
>
> Yes.
>>
>> Should these extra definitions live in the shared "datasource config" technical extension? Or can they be split?
>
> They should be split.  Each property would only live within its defining extension: generalized datasource support, generalized blocking and/or reactive support, specific driver support.
>>
>> we most likely need to allow raw JDBC URLs as well. There are many cases where passing a property via the URL is a lifesaver in case the upper framework does not expose some obscure feature of the database.
>
> This is something of a recurrent topic, isn't it?  And we vacillate quite a lot about our philosophy here.  I'm still opposed to configuration back doors.  I think it's better to add properties as needed to the per-driver extension.  Maybe even add them all up front.  But I'm not immovable on that.

I'm +1 to not allow "configuration backdoors", we should at least try
to stand our ground there.
Yet having the traditional JDBC URL allows a nice copy/paste
experience from documentation of databases, and other tools, so
there's at least some merit in allowing it - perhaps just veto any
property?

I don't like it - it seems like vetoing properties would be surprising to the user, even if we printed a warning, unless the warning specified exactly what properties were being overridden and how so that the user can update them.  And if we have that knowledge, then why not instead have some kind of external tool which creates a config blob out of a data source URL, and thus keep the configuration tight?

Merging properties has similar problems.

 

Another reason to separate at least the JDBC properties into ad-hoc
properties is that we'd be able to override the defaults from the JDBC
driver - in some cases they really should: not going to nit-pick now
on specific odd choices, let's keep it at the fact that having this
capability would be nice.

BTW - not sure who said this but it's worth clarifying - it's not
strictly true that all JDBC drivers connect to "some hostname"; some
of the newer storage toys are more PaaS oriented and as such use
different forms of identification encoded in the URL; but that's
simple to support of course, we'd just have different specific
properties for such weird ones :)
Just keep it in mind as the set of properties shared across all
databases is getting smaller.

"Shared" can mean more than one thing.  From the user's perspective, what it really means is "if I have to give a host name, I want the property name to be predictable".  It should not be too hard to make the host name be required for databases which require one, optional for databases which do not, and excluded (forbidden) for databases for which host name makes no sense.  I think this most closely matches the user's intuition.  Host name is not driver-specific by any reasonable understanding that a user would have, so it should not have N different configuration keys for N different drivers.

If it means we don't end up having a shared "data source config" class, I don't think that implementation detail matters too much, particularly from the user's perspective.

--

- DML

[Sanne Grinovero]

On Fri, 3 Jan 2020 at 18:18, David Lloyd <david...@redhat.com> wrote:
>
>
>
> On Fri, Jan 3, 2020 at 12:02 PM Sanne Grinovero <sa...@hibernate.org> wrote:
>>
>> On Fri, 3 Jan 2020 at 12:58, David Lloyd <david...@redhat.com> wrote:
>> >
>> > On Fri, Jan 3, 2020 at 6:33 AM Emmanuel Bernard <eber...@redhat.com> wrote:
>> >>
>> >> People feel it is ease of use compliant? I have to admit, my initial reaction was that it was complicated but I held off initial judgement. And it's getting better on me.
>> >>
>> >> First, aside from the fairly specific Flyway + reactive DB access where sharing is making things easier, I do think more and more that reactive and JDBC datasources are different and significantly alter how one writes an app. So not sharing the definition is ok for me. Of course if we share and see no drawback, even better.
>> >>
>> >> With that said, couple of questions / remarks:
>> >>
>> >> I see you set specific postgresql settings. Will I be able to add more and specifically more databases?
>> >
>> > Yes.
>> >>
>> >> Should these extra definitions live in the shared "datasource config" technical extension? Or can they be split?
>> >
>> > They should be split. Each property would only live within its defining extension: generalized datasource support, generalized blocking and/or reactive support, specific driver support.
>> >>
>> >> we most likely need to allow raw JDBC URLs as well. There are many cases where passing a property via the URL is a lifesaver in case the upper framework does not expose some obscure feature of the database.
>> >
>> > This is something of a recurrent topic, isn't it? And we vacillate quite a lot about our philosophy here. I'm still opposed to configuration back doors. I think it's better to add properties as needed to the per-driver extension. Maybe even add them all up front. But I'm not immovable on that.
>>
>> I'm +1 to not allow "configuration backdoors", we should at least try
>> to stand our ground there.
>> Yet having the traditional JDBC URL allows a nice copy/paste
>> experience from documentation of databases, and other tools, so
>> there's at least some merit in allowing it - perhaps just veto any
>> property?
>
>
> I don't like it - it seems like vetoing properties would be surprising to the user, even if we printed a warning, unless the warning specified exactly what properties were being overridden and how so that the user can update them. And if we have that knowledge, then why not instead have some kind of external tool which creates a config blob out of a data source URL, and thus keep the configuration tight?
>
> Merging properties has similar problems.

Ok so you're saying we should not allow people to paste a JDBC URL
from an existing document / tool at all?

I guess I can live with that, at least it's very clear that we'll be
in charge of setting "the right properties" based on other
configuration settings.

>> Another reason to separate at least the JDBC properties into ad-hoc
>> properties is that we'd be able to override the defaults from the JDBC
>> driver - in some cases they really should: not going to nit-pick now
>> on specific odd choices, let's keep it at the fact that having this
>> capability would be nice.
>>
>> BTW - not sure who said this but it's worth clarifying - it's not
>> strictly true that all JDBC drivers connect to "some hostname"; some
>> of the newer storage toys are more PaaS oriented and as such use
>> different forms of identification encoded in the URL; but that's
>> simple to support of course, we'd just have different specific
>> properties for such weird ones :)
>> Just keep it in mind as the set of properties shared across all
>> databases is getting smaller.
>
>
> "Shared" can mean more than one thing. From the user's perspective, what it really means is "if I have to give a host name, I want the property name to be predictable". It should not be too hard to make the host name be required for databases which require one, optional for databases which do not, and excluded (forbidden) for databases for which host name makes no sense. I think this most closely matches the user's intuition. Host name is not driver-specific by any reasonable understanding that a user would have, so it should not have N different configuration keys for N different drivers.
>
> If it means we don't end up having a shared "data source config" class, I don't think that implementation detail matters too much, particularly from the user's perspective.

Sure, I just meant to point this out. Database configuration is hardly
a standardized solved issue :)

>
> --
> - DML

>
> --
> You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CANghgrQeFGnHhDFjO8q00biaVZSR2336Lp1cZ5j_gaYO8VernA%40mail.gmail.com.

[Emmanuel Bernard]

Even in Hibernate which has a lot of JDBC properties available, I had a
few cases where passing properties via the connection URL was necessary.
Do I don't think we can over all the grounds that David was hoping for
to avoid the passthrough option of giving the URL (as a backup).

[Sanne Grinovero]

Thanks for translating :)

So far we operated with the mantra of "don't expose by default",
to only expose exotic properties when there's evidence for need.

I think we could try stiking to that in this case as well, at least as
starting point.

Again remember for sake of usability it would be great to set several
properties by default - but doing so would violate the principle of
least surprise if these are not explicitly listed among the
configuration properties.

I didn't build a census yet, but considering JDBC is an old standard
my expectation is that what you say might no longer apply: with many
JDBC properties, either they are no longer useful, or they exist
because flipping the default for an existing application wasn't
compatible - even though a new better default value might exist.