Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't force flush mode to FlushMode.ALWAYS #41284

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

yrodiere
Copy link
Member

Leave the flush mode to its default value in Hibernate ORM, which should smartly flush when necessary, and no more.

Discussed with more knowledgeable people here, and this seems like the right thing to do. I wouldn't backport though, as you can expect some behavior changes -- though mostly (only?) for the better.

Fixes #41115

Leave the flush mode to its default value in Hibernate ORM, which should
smartly flush when necessary, and no more.
@quarkus-bot quarkus-bot bot added area/hibernate-orm Hibernate ORM area/persistence OBSOLETE, DO NOT USE labels Jun 18, 2024
Copy link

quarkus-bot bot commented Jun 18, 2024

/cc @gsmet (hibernate-orm)

Copy link

quarkus-bot bot commented Jun 18, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 6c03c72.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

  • Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

  • Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

📦 integration-tests/reactive-messaging-kafka

io.quarkus.it.kafka.KafkaConnectorTest.testFruits - History

  • Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
	at io.quarkus.it.kafka.KafkaConnectorTest.testFruits(KafkaConnectorTest.java:63)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

@gsmet gsmet merged commit 85425b5 into quarkusio:main Jun 18, 2024
41 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jun 18, 2024
@yrodiere yrodiere deleted the i41115-default-flush-mode branch June 21, 2024 09:14
@famod
Copy link
Member

famod commented Jul 22, 2024

Hey @yrodiere, I'm currently testing 3.13.0.CR1 and I had to switch from .save() (using quarkus-spring-data-jpa) to .saveAndFlush().
Both cases I found so far failed to execute an update query via @Query (and @Modifying) updating a foreign key of A referencing B right after B was saved/created.

I wonder whether there should be a note in the migration guide?

@yrodiere
Copy link
Member Author

yrodiere commented Jul 22, 2024

Hey,

Hey @yrodiere, I'm currently testing 3.13.0.CR1 and I had to switch from .save() (using quarkus-spring-data-jpa) to .saveAndFlush().
Both cases I found so far failed to execute an update query via @Query (and @Modifying) updating a foreign key of A referencing B right after B was saved/created.

That's... interesting. And seems to point at a bug in Hibernate ORM, which maybe doesn't auto-flush before foreign key updates. At the very least this looks like a limitation, which the docs currently fail to mention (unless you're executing a native query?).

Could you please create an issue with a reproducer based on this template? We might want the Hibernate ORM team to have a look.

using quarkus-spring-data-jpa

To clarify, are we just talking about a regression in Quarkus, or are we also talking about a behavior that used to match that of Spring Data, and no longer does?

I'd expect Spring Data to behave the same as we now do, to be honest... FlushMode.ALWAYS was pretty aggressive.

I wonder whether there should be a note in the migration guide?

Definitely, will do. Thanks for pointing this out.
EDIT: Done.

@famod
Copy link
Member

famod commented Aug 5, 2024

@yrodiere thanks for enhancing the migration guide.

unless you're executing a native query?

No, it's a JPQL query (AFAICS): https://quarkus.io/guides/spring-data-jpa#user-defined-queries

To clarify, are we just talking about a regression in Quarkus, or are we also talking about a behavior that used to match that of Spring Data, and no longer does?

Haven't checked native Spring. It's a regression in the first place.

Could you please create an issue with a reproducer based on this template? We might want the Hibernate ORM team to have a look.

I was able to reproduce the issue with the Quarkus spring-data-jpa-quickstart (which I'll push soon).
For now, I ran out of time to create a Quarkus-free reproducer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hibernate batch-fetching seems to be broken in some situations where it works with pure Hibernate
3 participants