HHH-15942 introduce ForcedFlushMode #5851

gavinking · 2022-12-28T14:32:53Z

for specifying whether a query flushes or not:

replaces FlushModeType in the annotation package
much less confusing when applied to a Query
- what do MANUAL and COMMIT mean for a Query?
- how is AUTO useful for a Query?
also make Query.getHibernateFlushMode() obey its documented semantics by returning the session flush mode instead of null when unset

…ushes or not - replaces FlushModeType in the annotation package - much less confusing when applied to a Query * what do MANUAL and COMMIT mean for a Query? * how is AUTO useful for a Query? - also make Query.getHibernateFlushMode() obey its documented semantics by returning the session flush mode instead of null when unset

hibernate-core/src/main/java/org/hibernate/boot/model/internal/QueryBinder.java

 				.setTimeout( namedNativeQuery.timeout() < 0 ? null : namedNativeQuery.timeout() )
 				.setFetchSize( namedNativeQuery.fetchSize() < 0 ? null : namedNativeQuery.fetchSize() )
-				.setFlushMode( getFlushMode( namedNativeQuery.flushMode() ) )
+				.setFlushMode( getFlushMode( namedNativeQuery.flush(), namedNativeQuery.flushMode() ) )


hibernate-core/src/main/java/org/hibernate/boot/model/internal/QueryBinder.java

 				.setTimeout( namedQuery.timeout() < 0 ? null : namedQuery.timeout() )
 				.setFetchSize( namedQuery.fetchSize() < 0 ? null : namedQuery.fetchSize() )
-				.setFlushMode( getFlushMode( namedQuery.flushMode() ) )
+				.setFlushMode( getFlushMode( namedQuery.flush(), namedQuery.flushMode() ) )


gavinking · 2022-12-28T15:33:59Z

hibernate-core/src/main/java/org/hibernate/ForcedFlushMode.java

+	 * to flush, depending on its {@link FlushMode}.
+	 */
+	NO_FORCING
+}


Everyone, please take a look at this new thing!

Given that this is solely about queries, I'd be more inclined to name this QueryFlushMode or move it into o.h.query. For QueryFlushMode, I'd go with:

enum QueryFlushMode { FLUSH, SKIP, /** * Defer to {@link Session#getHibernateFlushMode} */ AUTO }

I get why you named it as you did. I just find it confusing

QueryFlushMode is fine by me, it was also my first thought.

You're right, I agree it belongs in o.h.query.

FLUSH is fine.

SKIP I don't find extremely clear though I could live with it.

I don't like AUTO here because it doesn't have the same semantics as FlushMode.AUTO.

sebersole · 2023-01-04T18:51:37Z

what do MANUAL and COMMIT mean for a Query?

Both of those are pseudonyms for never wrt a Query

how is AUTO useful for a Query?

AUTO is actually the default for a Query. Not sure what you are asking there tbh

sebersole · 2023-01-04T18:54:32Z

So iiuc this basically adds a whole new enum that just limits the set of values from another enum for use in a specific context.

gavinking · 2023-01-04T18:55:46Z

Both of those are pseudonyms for never wrt a Query

Yes of course, I know that, but it's nonobvious to a casual onlooker.

AUTO is actually the default for a Query. Not sure what you are asking there tbh

What I'm saying is there's too many things there and it's hard for the user to pick the right one. If I'm going to set this at the level of a query (rather than at the level of the session, where all of the values are useful) then I'm setting it because I either want to:

force a flush, or
force it not to flush.

sebersole · 2023-01-04T18:56:00Z

hibernate-core/src/main/java/org/hibernate/query/Query.java

 	// covariant overrides - CommonQueryContract

 	@Override
 	Query<R> setHibernateFlushMode(FlushMode flushMode);


Is there a reason this is not deprecated? It seems to me the intent here is to replace FlushMode when used in the context of a Query

I could deprecate it if you prefer.

gavinking · 2023-01-04T19:00:02Z

So iiuc this basically adds a whole new enum that just limits the set of values from another enum for use in a specific context.

It replaces org.hibernate.annotations.FlushModeType, which takes all the values of org.hibernate.FlushMode, and adds an additional value.

gavinking · 2023-01-04T19:01:02Z

I mean, to clarify, my first draft of this was to just deprecate FlushModeType with no replacement, but then I got to thinking about it and decided that this was nicer.

Sanne · 2023-01-13T10:09:52Z

Should we clarify how and if such new options are affected by the configuration properties?

For example we have property org.hibernate.flushMode, I think at least its documentation should say something about how it interacts with the "query flush modes".

gavinking · 2023-01-13T11:52:49Z

@Sanne you mean the query hint? That is unaffected by this proposal because it's mostly a JPA thing. When using Hibernate-native APIs we don't need hints.

I think there's also a config property but IIRC it's just a default value for that hint. (And I don't consider it a useful thing.)

This proposal is very much only affecting the native APIs.

Sanne · 2023-01-13T20:09:07Z

I'm referring to the configuration property AvailableSettings#FLUSH_MODE, AFAIK it changes the FlushMode, impacting all Session(s) - I'm not sure what you mean by "mostly a JPA thing"?

If they have no relation that's totally fine, but I think that should be made clear.

gavinking · 2023-01-13T21:17:35Z

I'm referring to the configuration property AvailableSettings#FLUSH_MODE, AFAIK it changes the FlushMode, impacting all Session(s) - I'm not sure what you mean by "mostly a JPA thing"?

Yeah, OK, you're right, it is actually a session-level setting, not a query hint, even though:

it looks like a query hint, since it doesn't satisfy our naming convention for configuration properties, and
it's only documented as a query hint.

Actually I think we should deprecate this thing, I don't see how it's useful, and honestly it almost looks like it got there by accident.

If they have no relation that's totally fine, but I think that should be made clear.

This proposal has no relation at all to the session-level flush mode, that's correct.

gavinking · 2023-01-13T21:48:38Z

Actually I think we should deprecate this thing

#5954

Actually it's fine, as a EntityManager-level JPA hint, which is what it started out as. It just should exist as a Hibernate config property in AvailableSettings.

gavinking · 2024-09-14T22:33:08Z

superseded by #8954

gavinking force-pushed the forcedflushmode branch from 436e4bf to 6e0def0 Compare December 28, 2022 14:48

github-advanced-security bot found potential problems Dec 28, 2022

View reviewed changes

gavinking commented Dec 28, 2022

View reviewed changes

sebersole reviewed Jan 4, 2023

View reviewed changes

gavinking mentioned this pull request Sep 14, 2024

HHH-15942 introduce QueryFlushMode #8954

Merged

gavinking closed this Sep 14, 2024

Uh oh!

HHH-15942 introduce ForcedFlushMode #5851

HHH-15942 introduce ForcedFlushMode #5851

Uh oh!

Conversation

gavinking commented Dec 28, 2022

Uh oh!

Check notice

Check notice

gavinking Dec 28, 2022

Choose a reason for hiding this comment

Uh oh!

sebersole Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

gavinking Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

sebersole commented Jan 4, 2023

Uh oh!

sebersole commented Jan 4, 2023

Uh oh!

gavinking commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebersole Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

gavinking Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

gavinking commented Jan 4, 2023

Uh oh!

gavinking commented Jan 4, 2023

Uh oh!

Sanne commented Jan 13, 2023

Uh oh!

gavinking commented Jan 13, 2023

Uh oh!

Sanne commented Jan 13, 2023

Uh oh!

gavinking commented Jan 13, 2023

Uh oh!

gavinking commented Jan 13, 2023

Uh oh!

gavinking commented Sep 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gavinking commented Jan 4, 2023 •

edited

Loading

gavinking commented Sep 14, 2024 •

edited

Loading