Hibernate ORM extension config overhaul #1121

gsmet · 2019-03-01T10:23:32Z

@Sanne here what I came up with.

I tried to organize properties a bit. The doc update might be a good starting point to have a general overview.

Still on my TODO list:

enforce default values for some properties
change the cache tests and documentation to use the Map syntax (i.e. using double quotes around the cache name - we won't use Maps at first but we can at least make the syntax stable)
make most of them runtime configuration
support multiple persistence units once we have support for multiple datasources (waiting for the Config Map fix)

I'll try to get the first two in before the release.

gsmet · 2019-03-01T10:24:32Z

docs/src/main/asciidoc/hibernate-orm-guide.adoc

+The maximum size of the query plan cache.
+
+`quarkus.hibernate-orm.query.batch-fetch-size`:: (defaults to `-1` i.e. batch fetching is disabled).
+The size of the batches used when loading entities and collections.


This one I'm not totally sure about. It's not exactly about queries but about the loaders but having a loader category would be very specific.

I'm not sure. Either query, tuning or loaders works for me.

tuning could be a choice but that's assuming we'll eventually have more such options? Also possibly too generic.

loaders is the most appropriate, but I doubt we'll have more, so hard justifies having a "category".

Maybe shouldn't be a category, this is getting too deep already? Just quarkus.hibernate-orm.batch-fetch-size ?

Yeah, it's not clear that people have to understand the distinction between query and jdbc batch size settings, or is it important?

They are totally different things and used to tune totally different things.

I can see how it is not easy to see the distinction but that's why there is a jdbc category. Because it's really about fine-tuning the behavior of the JDBC driver.

-1 for jdbc, it's not like we're reconfiguring the jdbc driver, we're just loading things in efficient blocks.. so that would be confusing.

Let's go with quarkus.hibernate-orm.batch-fetch-size ? N.B. it's not used only for queries, it's a more global, general tuning.

I was not suggesting using jdbc for batch-fetch-size. I was talking about the JDBC fetch/batch settings Stéphane referred to. They are entirely different things.

batch-fetch-size is only used for loading entities/collections.

I can get it out of a category though.

Sanne · 2019-03-01T10:46:48Z

docs/src/main/asciidoc/hibernate-orm-guide.adoc

+Default precedence of null values in `ORDER BY` clauses.
+Options are `none`, `first`, `last`.
+
+`quarkus.hibernate-orm.transaction.isolation-level`::


So let's remove this one as it needs to be handled by Agroal.

P.S. The good thing about removing this one is that it didn't fully belong under "Query" section either

FroMage · 2019-03-01T15:54:17Z

docs/src/main/asciidoc/hibernate-orm-guide.adoc

+The default catalog to use for the database objects.
+
+`quarkus.hibernate-orm.schema.default-schema`::
+The default schema to use for the database objects.


Would quarkus.hibernate-orm.schema and quarkus.hibernate-orm.catalog make sense for the defaults to use?

+1 yeah that sounds quite better actually

I don't think it's a good idea. Especially for schema as we have schema.generation. And they are different things.

Having the default prefix helps to understand what it does IMHO.

well it's not like one can switch schema at runtime.. so what is the "default" for?

AFAIK the only case is one can override the schema for a specific entity; I did that a couple of times but TBH I don't remember if that was a hack..

schema.default-schema looks like something went wrong, though, which is why I brought this up.

Well, unfortunately, the same name is used for the database schema and the notion of schema in a database.

I renamed .schema. to .database. to make things hopefully clearer and a few other adjustments to get something hopefully better.

FroMage · 2019-03-01T15:55:10Z

docs/src/main/asciidoc/hibernate-orm-guide.adoc


+==== JDBC
+
+`quarkus.hibernate-orm.jdbc.timezone`::


Do we have more than one timezone setting? Does it matter that it is used via JDBC?

Not at the moment. I must admit I wondered if we should just have quarkus.hibernate-orm.timezone.

I just thought being cautious wouldn't hurt here as what it does is pretty clear.

let's keep the jdbc prefix as it clarifies the scope.

Not sure if it shouldn't just be jdbc-timezone ? I wonder if we're not abusing the period: should be reserved for sub-categories?

Well. It may clarify the scope for people who understand what that scope means. If you think it's important that users understand that it's jdbc then fine. If this timezone thing would be relevant to users no matter if they need to know if it's jdbc or not, then it's wrong.

I personally don't know enough about its function to judge, so it's a honest question.

It's the timezone used by the JDBC driver, which might be different of the timezone of the JVM.

So it's important to know it's the one from the JDBC driver.

FroMage · 2019-03-01T15:57:12Z

docs/src/main/asciidoc/hibernate-orm-guide.adoc

+`quarkus.hibernate-orm.log.sql`:: (defaults to `false`).
+Show SQL logs and format them nicely.
+
+`quarkus.hibernate-orm.log.jdbc-warnings`:: (defaults to `false`).


Do we expect more kinds of warnings? If not how about log.warnings?

They are really specific to JDBC and the capability of the driver to get them.

So having warnings wouldn't make sense.

So it really is important to single out jdbc warnings from non-jdbc warnings?

Again, I've no clue, so it's a honest user question. I sort of assumed that hibernate had warnings, and that jdbc warnings would be a category within them.

they are different. Essentially this allows the Database to issue warnings which we relay to the application log.. sometimes that's very useful as the DB can give you some very good clues, but if you're not opting in specifically, the fact that we do this is really weird.

So you collect jdbc/db warnings and log them in the ORM logger somewhere. From a clueless user perspective, it sounds like something I would force to true (and set log level to debug) and let users decide if they want to see them by bumping up those log's category.

Apparently, this things is there because you could have weird issues depending on your driver/database and you need some knob to be able to disable it/enable it if needed.

Ideally, it's something that should be managed automatically but that's not the case right now.

Apparently, it was judged sufficiently important by the Hibernate ORM team to get to the Hibernate ORM - Quarkus config document so let's not have this discussion again.

FroMage · 2019-03-01T15:57:30Z

docs/src/main/asciidoc/hibernate-orm-guide.adoc

+
+==== Statistics
+
+`quarkus.hibernate-orm.statistics.enabled`:: (defaults to `false`)


I assume these statistics are not going via the logs then?

It's the collection of statistics that is enabled.

OK, so they're collected in some API. Would quarkus.hibernate-orm.statistics (defaults to 'disabled/false') not be enough?

Do we have a convention on config keys that end with .enabled? Sounds like we should really make feature toggles consistent across every extension and document those consistency rules somewhere.

good points.

I used that because I suspect we might want to have some more fine-grained statistics tuning in the future. I suppose we could shoe it in later.

See #560.

It has to be done in Agroal config as it is the pool managing that.

gsmet · 2019-03-02T14:23:14Z

@Sanne @FroMage I tried to address most of your comments in follow-up commits with specific commit messages so that you can quickly see what has been done.

I think it's better now.

About categories, the idea is to structure the configuration a bit as I'm pretty sure we will add more in the future. Hopefully, it will help with consistency and clarity in the long run.

Sanne · 2019-03-03T14:22:47Z

Looks great 💯 Let's merge this, perfection can be achieved later

Sanne · 2019-03-03T14:23:39Z

P.S. we'll need to update the properties used in demos and quickstarts. I might do that later today, but first need to finish the cache release.

Sanne · 2019-03-03T14:25:13Z

Created #1158

* fix: Make sure jbang bash script can update on Windows Now that we no longer delegate to the jbang .cmd file when running inside a Bash shell on Windows the bash script needs to be able to perform the Windows update procedure. * fix: `jbang edit` using wrong paths on Windows Another regression now that we no longer delegate to CMD when using a bash environment on Windows.

gsmet commented Mar 1, 2019

View reviewed changes

Sanne self-assigned this Mar 1, 2019

Sanne reviewed Mar 1, 2019

View reviewed changes

Sanne approved these changes Mar 1, 2019

View reviewed changes

FroMage reviewed Mar 1, 2019

View reviewed changes

gsmet added 8 commits March 2, 2019 14:10

Hibernate ORM - Rename hibernate config root to hibernate-orm

a25e884

See #560.

Hibernate ORM - Rename a variable for more clarity

683b308

Hibernate ORM - Rename configuration properties and add more

63393ef

Hibernate ORM - Update documentation

4cd8f58

Hibernate ORM - Remove the isolation level configuration

5de9447

It has to be done in Agroal config as it is the pool managing that.

Hibernate ORM - Move batch-fetch-size at the root

e6ddab9

Hibernate ORM - Don't make statistics a category

087b62e

Hibernate ORM - Rename the schema category to database

ca17c6a

Sanne merged commit dc64f22 into quarkusio:master Mar 3, 2019

Sanne deleted the hibernate-orm-config branch March 3, 2019 14:23

Sanne mentioned this pull request Mar 3, 2019

Update Hibernate quickstarts and examples to new properties #1158

Closed

gsmet added this to the 0.11.0 milestone Mar 3, 2019

emmanuelbernard removed coding-in-progress labels Mar 5, 2019


		==== Statistics

		`quarkus.hibernate-orm.statistics.enabled`:: (defaults to `false`)

Hibernate ORM extension config overhaul #1121

Hibernate ORM extension config overhaul #1121

Uh oh!

Conversation

gsmet commented Mar 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sanne Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsmet Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsmet commented Mar 2, 2019

Uh oh!

Sanne commented Mar 3, 2019

Uh oh!

Sanne commented Mar 3, 2019

Uh oh!

Sanne commented Mar 3, 2019

Uh oh!

Uh oh!

Sanne Mar 1, 2019 •

edited

Loading

gsmet Mar 1, 2019 •

edited

Loading