Steve, can you attach this as a patch against 1.7.x on the relevant ticket, for code review and inclusion?
(If you can comment on how much testing you've done after making this fix, that'd be helpful. Surely it'll be 1000% more than the testing anyone else has done on logic.) -Darius On Mon, Aug 1, 2011 at 10:23 AM, sjmckee <[email protected]> wrote: > So is the decision to remove the transactional annotation from the Logic > Service interface completely or is it to just remove it from the eval > methods? Who is going to be responsible for the change? CHICA?**** > > ** ** > > Also just as an FYI, I pulled this from the Spring site about transactional > annotations on interfaces:**** > > ** ** > > “The Spring team's recommendation is that you only annotate concrete > classes with the @Transactional annotation, as opposed to annotating > interfaces. You certainly can place the @Transactional annotation on an > interface (or an interface method), but this will only work as you would > expect it to if you are using interface-based proxies. The fact that > annotations are *not inherited* means that if you are using class-based > proxies then the transaction settings will not be recognized by the > class-based proxying infrastructure and the object will not be wrapped in a > transactional proxy (which would be decidedly *bad*). So please do take > the Spring team's advice and only annotate concrete classes (and the methods > of concrete classes) with the @Transactional annotation. “**** > > ** ** > > Thanks,**** > > Steve**** > > ** ** > > *From:* Darius Jazayeri-3 [via OpenMRS Mailing List Archives] [mailto: > ml-node+[hidden email]<http://user/SendEmail.jtp?type=node&node=6641812&i=0>] > > *Sent:* Friday, July 29, 2011 12:19 PM > *To:* McKee, Steven Jay > *Subject:* Re: Logic Transactional Annotations**** > > ** ** > > Tammy,**** > > ** ** > > I'm fine getting rid of the @Transactional on LogicService.eval. (We'd want > to do this in 1.6.x, 1.7.x, 1.8.x, and trunk.)**** > > ** ** > > But we should pair this with adding @Transactional(readOnly=true) on the > relevant method in LogicObsDAO (and other LogicDAOs in the logic module) to > address Roger's concern.**** > > ** ** > > (Even if the latter doesn't get done, I think the likelihood of experience > non-repeated reads is vanishingly small. Since properly-written logic rules > fetch data once for their whole cohort of patients, and cache that in the > logic context.)**** > > ** ** > > -Darius**** > > ** ** > > On Fri, Jul 29, 2011 at 7:57 AM, Friedman, Roger (CDC/CGH/DGHA) (CTR) <[hidden > email] <http://user/SendEmail.jtp?type=node&node=6634100&i=0>> wrote:**** > > Is Transactional necessary to assure repeatable read? That is, suppose we > have multiple conditions based on obs; we want to make sure that each > condition is evaluated against the same version of obs notwithstanding any > updates done closely in time.**** > > **** > > *From:* [hidden > email]<http://user/SendEmail.jtp?type=node&node=6634100&i=1>[mailto:[hidden > email] <http://user/SendEmail.jtp?type=node&node=6634100&i=2>] *On Behalf > Of *Tammy Dugan > > *Sent:* Friday, July 29, 2011 10:24 AM > *To:* [hidden email]<http://user/SendEmail.jtp?type=node&node=6634100&i=3> > > *Subject:* Re: [OPENMRS-DEV] Logic Transactional Annotations > **** > > **** > > Ok. Is the consensus then that we can take the transactional annotation off > of the LogicService in 1.7.x? > > Thanks, > > Tammy > > On 7/29/2011 10:02 AM, Burke Mamlin wrote: **** > > Tammy is correct, any evaluation or data fetches within logic rules are > supposed to use the LogicContext rather than going back to the API. This is > to preserve the context in which evaluations are performed (e.g., the index > date) as well as allow for context-sensitive caching. **** > > **** > > By design, logic rules are not expected to have side effects (e.g., write > to the database). Logic was designed to provide data and allow application > (outside of logic, using logic) to perform actions. To that end, I don't > know that logic needs to have any transactional annotation. If someone > wants to write to a database within a rule, then those should be done within > their own transaction (i.e., within the scope of that rule) and shouldn't > affect the logic process (the eval in which they're being performed).**** > > **** > > -Burke**** > > On Fri, Jul 29, 2011 at 9:41 AM, Tammy Dugan <[hidden > email]<http://user/SendEmail.jtp?type=node&node=6634100&i=4>> > wrote:**** > > All the calls are LogicService.eval(). Should they be LogicContext.eval? I > am assuming yes. > > Tammy **** > > > > On 7/28/2011 5:31 PM, Darius Jazayeri wrote: **** > > Okay, I don't know why transactions don't work the way I expect. But it may > be irrelevant. **** > > **** > > One specific question: the inner calls are done via LogicContext.eval, not > via LogicService.eval, right?**** > > **** > > Copying Burke here, for the conceptual question...**** > > **** > > To summarize, CHICA is calling one rule:**** > > * logic.eval("ScoreMCHAT"); // outer call**** > > **** > > Within that rule, they call 2 other rules:**** > > * logic.eval("CheckIncompleteScoringJit") // inner call 1**** > > * logic.eval("ScoreJit"); // inner call 2**** > > **** > > The behavior they *want* is that inner call 1 writes data to the DB, and > inner call 2 sees this. As things are currently set up, there's a > transaction wrapped around the *outer* call, so the db write they do in > inner call 1 is *not *visible in inner call 2. (I don't actually > understand why not.)**** > > **** > > The question is, can we remove the @Transactional wrapper around > logic.eval? (Only methods that actually write to the DB in the DAO layer, > will have a @Transactional annotation.) The case I worry about is if inner > call 2 throws an exception, we might *want* to rollback the transaction, > canceling the write from inner call 1.**** > > **** > > -Darius**** > > On Thu, Jul 28, 2011 at 12:41 PM, Tammy Dugan <[hidden > email]<http://user/SendEmail.jtp?type=node&node=6634100&i=5>> > wrote:**** > > From my testing, if you have nested transactions in daos with an outer > transaction on the service, the dao methods do NOT commit the transaction > until the outer transaction is finished, at least with the hibernate > configuration that openmrs currently has. I have had to get around this by > making any database update/insert that I execute as a side effect of a rule > be a Stateless session because hibernate does not flush the cache and > actually commit to the database. I have tried to explicitly flush and evict > objects but it doesn't seem to work. > > Thanks, > > Tammy **** > > > > On 7/28/2011 3:25 PM, Darius Jazayeri wrote: **** > > This surprises me, actually. @Transaction defaults to propagation=REQUIRED, > so these two **** > > logic.eval("CheckIncompleteScoringJit")**** > > logic.eval("ScoreJit");**** > > should happen *within* the existing transaction. Thus I assumed writes > that you do in the first eval call are visible in the second eval call.*** > * > > **** > > Is there a chance this is actually due to Hibernate not having flushed > things (i.e. actually fired the sql commands) rather than the transaction? > What if you added a Context.flushSession() between those calls?**** > > **** > > Or am I misunderstanding the way database transactions work?**** > > **** > > -Darius**** > > **** > > **** > > On Thu, Jul 28, 2011 at 11:07 AM, Tammy Dugan <[hidden > email]<http://user/SendEmail.jtp?type=node&node=6634100&i=6>> > wrote:**** > > Darius, > > Here is an example use case: > > * a nurse scans an MCHAT form > * the scanning of the MCHAT form triggers a rule that auto-scores the form > (ScoreMCHAT) > * within the mchat scoring rule (ScoreMCHAT) a rule is called to check to > see if the form is incomplete (CheckIncompleteScoringJit) > * if the form is incomplete, a patient state is stored called > JIT_incomplete in a database table for that form > * a second rule in the mchat scoring rule (ScoreMCHAT) sees if the form was > incomplete by looking for a JIT_incomplete state and if not scores it > (ScoreJit) > > > Therefore, we have: > > logic.eval("ScoreMCHAT"); > > within ScoreMCHAT we call: > > logic.eval("CheckIncompleteScoringJit") > logic.eval("ScoreJit"); > > With the transactional annotation on the logic.eval method, the > JIT_incomplete state does not exist in the database table when > logic.eval("ScoreJit") is called and the ScoreJit rule fails to see that the > form was incomplete. > > Thanks, > > Tammy **** > > > > > On 7/28/2011 1:53 PM, Darius Jazayeri wrote: **** > > Can you spell out an example a bit more explicitly? (I understand what > you're saying and asking, but I don't have a clear opinion at the > moment.) E.g what rule calls what other rule, such that the inner rule saves > data, and then the outer rule needs to be able to read it? **** > > **** > > We were mistaken ~5 years ago when we added the transactional annotations > on the service interface. I'd blame Ben, who actually wrote the code, but I > had even less of a clue at the time. :-) Anyway, the direction we'd like to > go is to move those transactional annotations to the specific DAO > implementation. I moved the annotations down in logic-0.5 as a trial run of > this. So in theory we may be able to remove that @Transactional annotation > on logic.eval, as you're asking. But I'd like to better understand whether > we really want to keep a single transaction wrapping the whole thing...e.g. > what if rule A calls rules B (successfully updates the db) and rule C > (exception!)? Should that roll back rule B's change? I'm not sure.**** > > **** > > -Darius**** > > On Thu, Jul 28, 2011 at 7:16 AM, Tammy Dugan <[hidden > email]<http://user/SendEmail.jtp?type=node&node=6634100&i=7>> > wrote:**** > > Darius, > > Because the transaction does not complete until the service method is > finished (because of the transactional annotation on the LogicService), we > have problems when nested rule calls try to save values to the database but > have later rules that depend on those saved values, before the transaction > commits. I have had to add in Stateless sections to get around this. Can we > reasonably remove the transactional annotation from the LogicService class > since transactions should be handled at the dao level anyway? > > Thanks, > > Tammy **** > > > > On 7/28/2011 10:13 AM, McKee, Steven Jay wrote: **** > > Darius,**** > > **** > > It has been a while since we discussed this, but hopefully I can refresh > your memory. The CHICA team is currently in the process of upgrading to > OpenMRS 1.7.x. We had multiple issues with transactional annotations on the > service layer when we attempted to upgrade to 1.6.x a while back. We moved > the annotations from the service layer to the DAO layer in our custom build > to get around a lot of errors that occurred. With 1.7.x we’re trying our > hardest to use OpenMRS directly and not have our own custom build.**** > > **** > > We noticed there have been transactional annotations added to the new > HibernateTokenDAO class in Logic 0.5.x. However, there’s still the > transactional annotation on the LogicService interface in core OpenMRS. We > just find it a little odd for transactions occurring on the service layer. > Tammy Dugan ran some tests and noticed transactions in the DAO layer do not > get committed until the method on the service layer completes. This occurs > even though the transactional annotations are on the methods in the DAO > layer class.**** > > **** > > Please let us know your thoughts on the annotations in the LogicService > interface.**** > > **** > > Thanks,**** > > Steve McKee**** > > **** > > *From:* [hidden email]<http://user/SendEmail.jtp?type=node&node=6634100&i=8> > [hidden > email] <http://user/SendEmail.jtp?type=node&node=6634100&i=9> *On Behalf > Of *Darius Jazayeri > > *Sent:* Thursday, January 27, 2011 12:17 PM > *To:* McKee, Steven Jay > *Cc:* Anand, Vibha; Dugan, Tammy Marie; Burke Mamlin; I Nyoman Winardi > Ribeka Pratihana > *Subject:* Re: Logic 0.5 for CHICA > **** > > **** > > Hi Steve,**** > > **** > > I created the branch for you ate > http://svn.openmrs.org/openmrs-modules/logic/branches/0.4.x-chica.**** > > **** > > How far along are you in the process of getting Chica upgraded to OpenMRS > 1.6? The reason I ask is because I made some pretty fundamental changes to > the logic module between 0.4 and 0.5, several of which may make your upgrade > job easier. (For example you can now store rules in a logic module database > table, provide an Arden language handler, and have them be automatically > recompiled as they change, without having to mess with any module-provided > classloaders So if you're still relatively far from having chica work on > 1.6, you might want to try going straight to the logic 0.5 version.**** > > **** > > -Darius**** > > On Wed, Jan 26, 2011 at 10:06 AM, Darius Jazayeri <[hidden > email]<http://user/SendEmail.jtp?type=node&node=6634100&i=10>> > wrote:**** > > Hi Steve,**** > > **** > > I moved some transactional annotations down to the DAO layer as well, so > that's the direction we're going with logic. That said, I'm not sure I want > to change this in the 0.4 branch because the point of that is to only have > minor bugfixes that won't possibly break anything for current users. I can > create you a new branch (0.4-chica or something) if you want to commit > there, instead of needing to manage a custom build.**** > > **** > > Regarding the equalTo method, can't use just use the .equals(Object) > method? (If we were to add .equalTo methods back into the LogicCriteria > interface, that will be really annoying, because I'll have to make those > changes in 1.6.x, 1.7.x, and 1.8.x, and it would need to happen before > Friday, when I plan to release 1.6.3.)**** > > **** > > -Darius**** > > **** > > On Wed, Jan 26, 2011 at 9:39 AM, Steven McKee <[hidden > email]<http://user/SendEmail.jtp?type=node&node=6634100&i=11>> > wrote:**** > > Darius, > > Thanks for the info! I was able to get the module deployed. > > I have a couple more questions concerning the 0.4 branch of logic. We are > currently targeting this version for our new production server (and > upgrading to 0.5 at a future time). There are a couple of changes we have > locally that we're not sure should go into the branch or not. I just want > to get your take on it. > > 1. @Transactional annotation. We were getting multiple unexpected rollback > exceptions a while ago. We moved the annotation from the > org.openmrs.logic.LogicService interface down to the > org.openmrs.logic.db.LogicRuleTokenDAO interface (should really go to the > HibernateLogicRuleTokenDAO class). It seems like the annotations should > normally be down at the DB level instead of the service anyways. > > 2. The org.openmrs.logic.LogicCriteriaImpl had a method removed when we > moved from OpenMRS 1.5 to 1.6 (0.4 of logic module). The method was > equalTo(Object o). Some of our Java rules are referencing the missing > method. We added the method back to LogicCriteria. We also had to add new > methods for each of the equalTo methods using primitives (int, float, etc.) > because some callers would go into the equalTo(Object o) method because of > objects being passed (Integer, Float, etc.). > > Just let us know your thoughts on the proposed changes we have. > > Thanks! > Steve**** > > > > On 1/25/2011 11:47 AM, Darius Jazayeri wrote: **** > > Hi Steven, **** > > **** > > I've changed things around so that instead of this:**** > > <property name="logicDataSources">**** > > <map>**** > > <entry key="RMRS"><ref bean="obsChicaDataSource" /></entry>**** > > <entry key="CHICA"><ref bean="logicObsDataSource" /></entry>**** > > </map>**** > > </property>**** > > **** > > you need to put the names directly in the data source classes, for example: > **** > > public static String NAME = "RMRS";**** > > **** > > Don't hesitate to ask more questions as you go.**** > > **** > > -Darius**** > > **** > > On Tue, Jan 25, 2011 at 6:56 AM, Steven McKee <[hidden > email]<http://user/SendEmail.jtp?type=node&node=6634100&i=12>> > wrote:**** > > Darius, > > I've been in charge of upgrading CHICA from OpenMRS 1.5 to 1.6. I've also > been asked to look into upgrading the logic module from 0.4 to 0.5. I'm > currently having issues getting CHICA to deploy in OpenMRS with the changes. > It looks like an issue with our moduleApplicationContext file. Running > against the 0.4 module the following works, but does not in 0.5. > > > <bean id="logicChicaObsDAO" > class="org.openmrs.modulechica.datasource.LogicChicaObsDAO"/> > > <bean id="obsChicaDataSource" > class="org.openmrs.module.chica.datasource.ObsChicaDatasource"> > <property name="logicObsDAO"><ref bean="logicChicaObsDAO"></ref></property> > </bean> > <bean parent="logicServiceTarget"> > <property name="logicDataSources"> > <map> > <entry key="RMRS"><ref bean="obsChicaDataSource" /></entry> > <entry key="CHICA"><ref bean="logicObsDataSource" /></entry> > </map> > </property> > </bean> > > > The following error occurs when I try to upload the module: > > nested exception is > org.springframework.beans.factory.BeanDefinitionStoreException: Invalid bean > definition with name 'logicServiceTarget$child#0' > defined in URL > [jar:file:/C:/Program%20Files/Apache%20Software%20Foundation/Tomcat%206.0/temp/1295963240882.openmrs-lib-cache/chica/chica.jar > !/moduleApplicationContext.xml]: Could not resolve parent bean definition > 'logicServiceTarget'; nested exception is > org.springframework.beans.factory.NoSuchBeanDefinitionException: No bean > named 'logicServiceTarget' is defined > > > > I noticed there have been changes in the moduleApplicationContext file for > the logic module, and the logicServiceTarget bean was removed from 0.4 to > 05. Any ideas on what I need to change to make it work with 0.5? > > > Thanks, > > -- > Steve McKee - Software Developer > Child Health Informatics Research and Development Lab > 410 West 10th Street, Suite 1020 > Indianapolis, IN 46202 > [hidden email] <http://user/SendEmail.jtp?type=node&node=6634100&i=13> > <a href="<a href="tel:%28317%29%20278-9660">tel:%28317%29%20278-9660" > target="_blank">(317) 278-9660**** > > **** > > **** > > **** > > **** > > **** > > **** > > -- **** > > Tammy Dugan**** > > CHIRDL Technical Lead**** > > Children's Health Services Research**** > > IU School of Medicine**** > > **** > > ** ** > > -- **** > > Tammy Dugan**** > > CHIRDL Technical Lead**** > > Children's Health Services Research**** > > IU School of Medicine**** > > **** > > ** ** > > -- **** > > Tammy Dugan**** > > CHIRDL Technical Lead**** > > Children's Health Services Research**** > > IU School of Medicine**** > > **** > > ** ** > > -- **** > > Tammy Dugan**** > > CHIRDL Technical Lead**** > > Children's Health Services Research**** > > IU School of Medicine**** > > **** > ------------------------------ > > [hidden email] <http://user/SendEmail.jtp?type=node&node=6634100&i=14>from > OpenMRS Developers' mailing list > **** > > ** ** > > -- **** > > Tammy Dugan**** > > CHIRDL Technical Lead**** > > Children's Health Services Research**** > > IU School of Medicine**** > > ------------------------------ > > [hidden email] <http://user/SendEmail.jtp?type=node&node=6634100&i=15>from > OpenMRS Developers' mailing list > **** > > ** ** > ------------------------------ > > [hidden email] <http://user/SendEmail.jtp?type=node&node=6634100&i=16>from > OpenMRS Developers' mailing list > **** > ------------------------------ > > *If you reply to this email, your message will be added to the discussion > below:* > > > http://openmrs-mailing-list-archives.1560443.n2.nabble.com/Re-Logic-Transactional-Annotations-tp6633720p6634100.html > **** > > To start a new topic under Developers, email [hidden > email]<http://user/SendEmail.jtp?type=node&node=6641812&i=1> > To unsubscribe from Developers, click here. **** > > ------------------------------ > View this message in context: RE: Logic Transactional > Annotations<http://openmrs-mailing-list-archives.1560443.n2.nabble.com/Re-Logic-Transactional-Annotations-tp6633720p6641812.html> > Sent from the Developers mailing list > archive<http://openmrs-mailing-list-archives.1560443.n2.nabble.com/Developers-f1490149.html>at > Nabble.com. > > ------------------------------ > Click here to > unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from > OpenMRS Developers' mailing list > _________________________________________ To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to [email protected] with "SIGNOFF openmrs-devel-l" in the body (not the subject) of your e-mail. [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l]
