Hibernate flush mode

[Rafal Korytkowski]

Hey,

I have been experimenting with Hibernate flush modes recently. The
motivation was that we experienced premature flushes triggered by
Hibernate while retrieving data from the DB, which made us temporarily
switch from FlushMode.AUTO to FlushMode.COMMIT or MANUAL to execute
some parts of code like validation.

The initial attempt for a more general solution was TRUNK-3069, which
switched from the AUTO to COMMIT mode for all transactions. The flush
was triggered by us around any method annotated with
@Transactional(readOnly=false), but not around
@Transactional(readOnly=true). It seemed like a good approach at
first, but then I discovered TRUNK-3103. The problem could be
eventually resolved by annotating with @Transactional dao methods,
which we are considering.

Anyway TRUNK-3069 disables much of Hibernate functionality to handle
flushes for us and now I think it is not how we should approach the
problem.

I believe we need to stay with the AUTO flush mode and tune it only
when it is needed. Unfortunately, we cannot change the flush mode in
any other place than the DAO layer where we have access to hibernate's
session, whereas most of the time we actually need to control it in
the service layer.

So far whenever we needed to execute a service method in the manual
flush mode our approach was to go down to the DAO layer, which
resulted in such strange constructions as in getDefaultConceptMapType
[0], where we put in the DAO layer code that really belonged to the
service layer.

We have a few possibilities to deal with that.

1. We continue to handle the flush issue in DAOs the way it was before.
2. We have something like CustomSessionFlushTask [1].
3. We have Context.getFlushMode() and Context.setFlushMode(flushMode).
We need our own FlushMode enum so that we don't introduce a dependency
on Hibernate in the service layer.
4. We have @ManualFlush annotation to annotate service methods that we
want to explicitly execute in the manual flush mode. It's a more
elegant variation of 2., but slightly less useful since it requires to
create a dedicated method. For instance we have the getConcept method
and if we want it to be executed in one place only in the manual flush
mode we need to create a second method getConceptInManualFlush for the
purpose of annotating it with @ManualFlush.

I am really curious what do you think or if there is anyone who has
more experience with that.

[0] - https://source.openmrs.org/browse/~br=1.9.x/OpenMRS/branches/1.9.x/api/src/main/java/org/openmrs/api/db/hibernate/HibernateConceptDAO.java?r=26243
[1] - https://source.openmrs.org/browse/~br=trunk/Modules/metadatasharing/trunk/src/org/openmrs/module/metadatasharing/api/db/hibernate/CustomSessionFlushTask.java?r=26268

-Rafal

[Mark Goodrich]

This may be completely unhelpful suggestion :) but as I've learned more about hibernate it seems like we've been making some basic invalid assumptions about how hibernate works. It would take a significant reworking, but would it be worth considering redoing the API so that we don't have to worry about the flush mode?

The change here would be we'd have to assume that whenever you change an object attached to a hibernate session, you are changing that object at the DB level, whether or not you call save or update.

For instance, the following unit test fails, but (correct me if I'm wrong... maybe I'm the only one that has been confused :) haven't we generally been writing code until the assumption that it would pass?

@Test
public void shouldDisplayDatePropertyAccessor() throws Exception {

Patient patient = Context.getPatientService().getPatient(2);
Assert.assertEquals("M", patient.getGender());

patient.setGender("F");

// don't call a save patient here

Context.closeSession();
Context.openSession();
authenticate();

Patient patient2 = Context.getPatientService().getPatient(2);
Assert.assertEquals("M", patient2.getGender());

}

Take care,
Mark

_________________________________________

To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to LIST...@LISTSERV.IUPUI.EDU with "SIGNOFF openmrs-devel-l" in the body (not the subject) of your e-mail.

[mailto:LIST...@LISTSERV.IUPUI.EDU?body=SIGNOFF%20openmrs-devel-l]

[Ben Wolfe]

Mark, thats a known issue and I think there is a ticket somewhere for that.  However, I thought it was if you called save for /any/ object it would save everything in the session.  This is a bug, but not something encountered often with how the api is used.

Rafal, another option that might be work exploring/spiking on is something that solves this flushing and Mark's point.  Detaching objects as they come out of hibernate and then reattaching when getting back in.  The problem to look out for is the places where we're depending on lazy loading. e.g. something like ${concept.creator.name} in jsp pages.  But thats probably has to wait for an API rewrite.

Of the short term solution suggestions, I like #4 the best.  Having the extra DAO methods shouldn't clutter the API because they're hidden down at the lower level.

Ben

[Friedman, Roger (CDC/CGH/DGHA) (CTR)]

Rafa --
Thanks for the spadework.
ConceptDAO contains a wealth of not so great ideas. I think working on REST has made us more aware of subclasses and helper classes and how to represent them.
Is it an option to have only the DAO layer deal with Hibernate-connected objects, to have it serve disconnected objects on read and reconnect them on write? Wouldn't that mean our services and the API would no longer be engaged with the session and its cache?
Saludos, Roger

-----Original Message-----
From: d...@openmrs.org [mailto:d...@openmrs.org] On Behalf Of Rafal Korytkowski
Sent: Friday, March 09, 2012 9:54 AM
To: openmrs...@LISTSERV.IUPUI.EDU
Subject: [OPENMRS-DEV] Hibernate flush mode

[Burke Mamlin]

Great summary, Mark.  I think we need to acknowledge that the API ("API 1.0") returns "magical" objects that are tied to the API and have the tendency to auto-save any changes.  I don't see any way to avoid this magic until we get to API 2.0, meaning I wouldn't expect the current API to support your unit test.  The question is, what guaranteed/predictable behavior for developers can we provide in the current API?  For example:

• Assume changes to domain objects retrieved from the API are persisted immediately.  In fact, changes may be persisted during any subsequent API call and are only guaranteed to be persisted with a call to save() the object or with a call to Context.closeSession().
• Always call save() after changing an object to (1) guarantee changes are persisted and (2) ensure your code is future-proofed should the "auto-save" behavior go away.

Can we gurantee that changes are persisted during a save() of an object?  I'd hope so.

-Burke

On Fri, Mar 9, 2012 at 11:02 AM, Mark Goodrich <mgoo...@pih.org> wrote:

[Rafal Korytkowski]

The problem in Mark's test is that when you call
Context.closeSession() within a transaction (our unit tests are
transactional by default) it only dettaches the session from the
transaction manager and doesn't close or even clear the session. It's
stored and attached again when you call Context.openSession(). If you
call Context.clearSession() before Context.closeSession() then it'll
work as you expect.

Roger, Ben explained the problem with dettaching objects from
Hibernate. Our domain objects are interconnected and lazily
initialized and we would have to initialize them prior to dettaching
from Hibernate. Sometimes a graph of objects that needs to be
initialized is quite vast and it could kill performance. It would
require some redesigning of our API and our business model classes,
which is quite a big change.

-Rafal

On 9 March 2012 17:50, Friedman, Roger (CDC/CGH/DGHA) (CTR)

[Mark Goodrich]

Ben,

 

But couldn’t it be argued that this is not a bug, but rather an error in the way we are using Hibernate?   I feel like we are going through a lot of hoops trying to get Hibernate to behave differently than the way it is intended to behave.

 

Mark

---

Click here to unsubscribe from OpenMRS Developers' mailing list

[Ben Wolfe]

Ah ha!  That explains a lot of confusing things in the past with unit tests and our sessions.  Seems like a bug that Context.closeSession() isn't clearing the session.  I can't think of a reason not to clear it unless there is some weird timing with closing it and flushing sessions with the normal webapp.

Ben

[Burke Mamlin]

Rafał,

If calls the API are made within the "// don't call a save patient here" excerpt in Mark's example, isn't it likely that the change will be persisted if Hibernate chooses to flush at any point?

It seems that both save() methods and Context.closeSession() should perform a flush – i.e., ensure Hibernate has a chance to save any unsaved changes.  Then we can at least provide a guarantee that a save() method or closeSession() will avoid persisting changes.

-Burke

On Fri, Mar 9, 2012 at 12:23 PM, Rafal Korytkowski <ra...@openmrs.org> wrote:

[Friedman, Roger (CDC/CGH/DGHA) (CTR)]

Rafal, this actually sounds like what we are talking about for custom representations in REST. We could pass around dynamically-created disconnected non-lazily-loaded datasets like
Patient(
Person(
DOB,
DOB_Estimated,
Gender),
Name(
Firstname
Middlename
Givenname),
Address(
Address1,
Address2,
Address3),
PatientID(
IDType(
Typename=="NationalID"),
IDText),
PatientAttributes(
AttributeType(
Typename=="Email"),
Value)
)
that would be used for all CRUD operations and then have services like "GetBestNameForLocale(concept,locale)".

-----Original Message-----
From: d...@openmrs.org [mailto:d...@openmrs.org] On Behalf Of Rafal Korytkowski

Sent: Friday, March 09, 2012 12:23 PM
To: openmrs...@LISTSERV.IUPUI.EDU
Subject: Re: [OPENMRS-DEV] Hibernate flush mode

[Mark Goodrich]

It just seems like the way our API is written and is used, the assumption is that changes made to a domain object are only persisted if a saveObject() service method is called. But by default Hibernate persists changes to any attached objects automatically, and we are struggling without complete success to stop "premature" flushes, which make the prior assumption false. I haven't read the API 2.0 notes, but my suggestion would be in 2.0 we give up on that assumption instead of trying to enforce it.

Mark
________________________________________
From: d...@openmrs.org [d...@openmrs.org] On Behalf Of Burke Mamlin [bma...@REGENSTRIEF.ORG]
Sent: Friday, March 09, 2012 12:54 PM

To: openmrs...@LISTSERV.IUPUI.EDU
Subject: Re: [OPENMRS-DEV] Hibernate flush mode

Rafał,

If calls the API are made within the "// don't call a save patient here" excerpt in Mark's example, isn't it likely that the change will be persisted if Hibernate chooses to flush at any point?

It seems that both save() methods and Context.closeSession() should perform a flush – i.e., ensure Hibernate has a chance to save any unsaved changes. Then we can at least provide a guarantee that a save() method or closeSession() will avoid persisting changes.

-Burke

On Fri, Mar 9, 2012 at 12:23 PM, Rafal Korytkowski <ra...@openmrs.org<mailto:ra...@openmrs.org>> wrote:
The problem in Mark's test is that when you call
Context.closeSession() within a transaction (our unit tests are
transactional by default) it only dettaches the session from the
transaction manager and doesn't close or even clear the session. It's
stored and attached again when you call Context.openSession(). If you
call Context.clearSession() before Context.closeSession() then it'll
work as you expect.

Roger, Ben explained the problem with dettaching objects from
Hibernate. Our domain objects are interconnected and lazily
initialized and we would have to initialize them prior to dettaching
from Hibernate. Sometimes a graph of objects that needs to be
initialized is quite vast and it could kill performance. It would
require some redesigning of our API and our business model classes,
which is quite a big change.

-Rafal

On 9 March 2012 17:50, Friedman, Roger (CDC/CGH/DGHA) (CTR)

> To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to LIST...@LISTSERV.IUPUI.EDU<mailto:LIST...@LISTSERV.IUPUI.EDU> with "SIGNOFF openmrs-devel-l" in the body (not the subject) of your e-mail.
>
> [mailto:LIST...@LISTSERV.IUPUI.EDU<mailto:LIST...@LISTSERV.IUPUI.EDU>?body=SIGNOFF%20openmrs-devel-l]

_________________________________________

To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to LIST...@LISTSERV.IUPUI.EDU<mailto:LIST...@LISTSERV.IUPUI.EDU> with "SIGNOFF openmrs-devel-l" in the body (not the subject) of your e-mail.

[mailto:LIST...@LISTSERV.IUPUI.EDU<mailto:LIST...@LISTSERV.IUPUI.EDU>?body=SIGNOFF%20openmrs-devel-l]

________________________________
Click here to unsubscribe<mailto:LIST...@LISTSERV.IUPUI.EDU?body=SIGNOFF%20openmrs-devel-l> from OpenMRS Developers' mailing list

[Wyclif Luyima]

I some how agree with Mark on his last comment, for me these don't seem like bugs as such but rather issues coming up probably because of  the way we are using hibernate.

I personally prefer option 3(Adding Context.set/getFlushMode) in Rafal's original email, with the default being Manual.

Wyclif

---

Click here to unsubscribe from OpenMRS Developers' mailing list

[Rafal Korytkowski]

Ben, I don't see a problem in clearing the session in our
Context.closeSession(). It'll work in Mark's case, but it won't fix
all the problems. If Hibernates chooses to flush the session before
you call Context.clearSesssion() and Context.closeSession(), the
changes will hit the db and when you call Context.openSession() and
try to retrieve the data you'll see them as if they were persisted
anyway as long as the transaction wasn't rolled back.

Burke, where do you find "ł" on your keyboard ;-) Hibernate chooses to
flush at very specific points. That is whenever you try to get
something from the db and it's not available in the session (cache) or
it's in the session, but marked as modified. It tries not to be eager
with flushes and does it only in cases when you access the same table
as you modified, but as reported here you cannot rely on that:
https://hibernate.onjira.com/browse/HHH-2399

In TRUNK-3069 I made flushes happen whenever you called save...,
void..., etc. and only then. At least it's when you would normally
expect them to happen. The drawback is still that all objects are
flushed not only the one you intend.

I don't like it anymore for the reason that it won't work with the MDS
module that needs exclusive control over the Hibernate flushes and it
won't be easy to make it work with that approach, unless I'll open a
back gate to disable Context.flushSession() ;-)

I also agree with Mark that we shouldn't be fighting with
Hibernate/JPA most of the time, but accept its pros and cons and maybe
consider detaching objects from Hibernate in API 2.0.

-Rafal

2012/3/9 Mark Goodrich <mgoo...@pih.org>:

> To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to LIST...@LISTSERV.IUPUI.EDU with "SIGNOFF openmrs-devel-l" in the  body (not the subject) of your e-mail.
>
> [mailto:LIST...@LISTSERV.IUPUI.EDU?body=SIGNOFF%20openmrs-devel-l]

[Burke Mamlin]

For API 2.0, we'd completely remove the magic of Hibernate outside of the API – e.g., return detached objects – sacrificing lazy loading for predictable, intuitive, and reliable behavior.

As it stands, I'm favoring that we (1) acknowledge how things work & strive to making the current behavior understandable/predictable instead of (2) trying to make it behave how we'd like with a caveat that "it might not work some of the time."

-Burke

2012/3/9 Mark Goodrich <mgoo...@pih.org>

[Burke Mamlin]

Rafał,

I have a launchbar script for your name (out of respect to you, of course). ;-)

I'd rather have an explicit, well-explained, and reliable API that doesn't work the way we would like (yet) instead of an API trying to be something that it isn't and, therefore, behave unexpectedly & unpredictabely.  For example, "Write code that saves changes, but assume changes to objects retrieved from the API may be persisted before you call save(); this will be fixed in the next major revision of the API."

Cheers,

-Burke

[Mark Goodrich]

Burke,

 

The thing is, I don’t know if we can change the API so it that doesn’t behave unexpectedly (sometimes) without a major revision to the API.

 

I don’t think we can 100% enforce that objects aren’t persisted until save is called unless we do a major rework. So do we say (and make a few tweaks if needed) “Assume that changes to objects retrieved from the API *are* persisted whenever a transaction ends or a session is closed, regardless of whether a save method is called; please disregard the fact that much of the existing code is written as if save methods need to be called explicitly” :)

 

Mark

---

Click here to unsubscribe from OpenMRS Developers' mailing list

[Burke Mamlin]

Mark,

I guess I'm not getting my point across.  Let me try a different way.  I'm proposing that we teach developers something like this:

Example 1:

Patient patient = Context.getPatientService().getPatient(2);

patient.setGender("F");

// assume gender is already persisted, but don't count on it (not guaranteed)

Context.getPatientService().savePatient(patient);

// change in gender is now guaranteed because of save

Example 2:

Context.openSession();

Patient patient = Context.getPatientService().getPatient(2);

patient.setGender("F");

// assume gender is already persisted, but don't count on it (not guaranteed)

// oops… I forgot to call savePatient() … this is bad practice.

Context.closeSession();

// change in gender is now guaranteed to be persisted, because

// closeSession() will guarantee unsaved changes

[Burke Mamlin]

(sorry about that, halfway through drafting my "clarification", gmail deciding to send my e-mail)

Mark,

I guess I'm not getting my point across.  Let me try a different way.  I'm proposing that we teach developers something like this:

Example 1:

Patient patient = Context.getPatientService().getPatient(2);

patient.setGender("F");

// assume gender is persisted, but don't count on it (not guaranteed)

Context.getPatientService().savePatient(patient);

// change in gender is now guaranteed because of save

Example 2:

Context.openSession();

Patient patient = Context.getPatientService().getPatient(2);

patient.setGender("F");

// assume gender is persisted, but don't count on it (not guaranteed)

// oops! forgot to call savePatient() … this is bad practice.

Context.closeSession();

// change in gender is now guaranteed to be persisted, because

// closeSession() will guarantee unsaved changes get saved;

// however, relying on this will ensure your code breaks when

// we go to API 2.0

By flushing in API save methods & in Context.closeSession, we guarantee changes get saved.  By recommending that people call API save methods after changes, we promote more robust (API 2.0-friendly) code.  Then our API docs just need to communicate something like:

"Changes to domain objects retrieved from the API may be autosaved between the time you make the change and when you invoke the API's save method, so don't make changes to domain objects retrieved from the API if you don't want those changes persisted.  You should not rely on this autosaving, but continue to call the API's save method both to guarantee changes are saved and to keep your code compatible with future versions of the API.  Autosaving (before the save method is called) is a confusing 'feature' of the API and will be removed in API 2.0."

I don't think we should be trying to convince ourselves or anyone else that we can change domain objects retrieved from the API and reliably avoid having those changes persisted.  On the other hand, we are not in a position to guarantee that autosaving will always happen and when/where it will happen, so the best we can do is explain the behavior and provide a gurantee of persistence following a save or closeSession.

Clear as mud?

[Wesley Brown]

My apologies for jumping into your discussion... 

I'm new to both OpenMRS and Hibernate (though familiar with NHibernate and the like on the .Net side), but the prospect of not knowing when an update to a domain object will actually get persisted to the db seems like it could lead to many subtle bugs without giving any benefit.  For something as important as writing to the database, I would always want to be able to deterministically determine when that update was going to occur.  I can think of three main reasons for this:

1. I might want to cancel an update after changing the domain object fields but before explicitly calling saveX().
2. I don't want to needlessly make a call to the db twice in this scenario:
1. Update domain object
2. Autosave occurs
3. More updates to the domain object
4. Call saveX();
3. How can one reliably confirm that the object data sent to the database is in a valid state if we don't know when that object will be persisted to the database?

It seems that the best behavior, in terms of simplicity and consistency, is to only persist a given domain object when the `saveX()` method is called.  This also implies that calling saveX() would NOT persist any changes to domain object Y as doing so would break that simplicity and consistency.

Are you saying that v2 of the API will only persist a domain object when saveX() is called but that the current version may do the autosave?

-Wes

[Burke Mamlin]

Wes,

Autosave is undesirable for the reasons you suggest, but it unfortunately exists in our current API, so we have to live with it.  Basically, since we took the bait of allowing lazy loading – not that we're lazy ;-) –  outside of the API, it means that the objects returned by the API are still attached to Hibernate and we suffer the autosaving side effect.

So, you're correct: we are saying that v2 of the API will only persist a domain object when saveX() is called (and will need to make properties explicit instead of relying on lazy loading), but the current version may do the autosave.

Cheers,

-Burke

[Mark Goodrich]

Burke—

 

That makes sense to me…