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

HHH-13677 Make org.hibernate.flushMode config take effect #3144

Closed

Conversation

NathanQingyangXu
Copy link
Contributor

@NathanQingyangXu NathanQingyangXu commented Dec 13, 2019

https://hibernate.atlassian.net/browse/HHH-13677

We ignored setting flush mode during session building. This PR simply grabs the setting from fastSessionService. Testing case added.

@NathanQingyangXu NathanQingyangXu changed the title HHH-13677 make org.hibernate.flushMode config take effect HHH-13677 Make org.hibernate.flushMode config take effect Dec 13, 2019
@NathanQingyangXu NathanQingyangXu force-pushed the HHH-13677 branch 5 times, most recently from cf49cb0 to 8affd32 Compare December 18, 2019 03:39
@NathanQingyangXu
Copy link
Contributor Author

Thank you, guys!

@@ -256,6 +256,8 @@ public SessionImpl(SessionFactoryImpl factory, SessionCreationOptions options) {
// NOTE : pulse() already handles auto-join-ability correctly
getTransactionCoordinator().pulse();

getSession().setHibernateFlushMode( ConfigurationHelper.getFlushMode( getSessionProperty( AvailableSettings.FLUSH_MODE ), FlushMode.AUTO ) );

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sanne should we add the FlushMode to the FastSessionServices?

Copy link
Contributor Author

@NathanQingyangXu NathanQingyangXu Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, FastSessionServices contains FlushMode default value. If you drill down the getSessionProperty() you can see it will look up in FastSessionServices by default. The FlushMode.AUTO above will never take effect for FastSessionServices should have returned a non-null value for AvailableSettings.FLUSH_MODE :
Screen Shot 2020-01-27 at 5 34 12 PM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, looks good. Is something not working as expected @dreab8 ? Feel free to add tests :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't noticed that getSessionProperty takes the value from the FastSessionServices

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hum both ConfigurationHelper#getFlushMode and the getSessionProperty methods are rather slow though. I see what @dreab8 meant now, I will add an additional fix to this.

@dreab8
Copy link
Contributor

dreab8 commented Jan 28, 2020

Applied upstream.
Thanks @NathanQingyangXu

@dreab8 dreab8 closed this Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants