> On Dec. 17, 2015, 8:36 a.m., Nate Cole wrote: > > I would say that if we're going to do this, then it should be to remove the > > member variable completely for getters and setters. Keeping them around in > > memory is what gets us into trouble. Any other classes where we follow > > this (anti)pattern? > > Jonathan Hurley wrote: > I agree - we need to remove the backing entity entirely and always get it > from the EM. We have to consider more than just "setters" here; anything that > touches things like OneToMany collections on the cluster entity also would > need to be considered a setter. We should just get rid of it entirely and > rely on the EM. > > Sid Wagle wrote: > @Nate: Yes we had this dettached entity reference caching problem in > ServiceImpl, ServiceComponentImpl and ServiceComponentHostImpl. > > @Jonathan: Agree with addressing the problem where related entites are > touched, in this patch I went after anything that does a *merge* on > clusterEntity, the other methods for example *applyLatestConfigurations* does > a merge on the related Collection entities and then a refresh() on the parent > clusterEntity which updates the reference, I don't think that would lead to a > problem because everything CASCADES down from cluster. Although, I will look > at this class again to make sure all such collection merges are covered. > > I still preserved reads from the dettached entites for things that rarely > or never change like *name* of Cluster / Service / Component. > Referencing changes done previously in this regard: > https://reviews.apache.org/r/39996/ > > Jonathan Hurley wrote: > If someone adds a new setMethod or a method that touches collections, > they'll also have to remember to refresh the cluster entity. Instead, if we > just retrieved it from the EM each time, we wouldn't need to worry about that > invalidation, right? Is there a downside to always getting it from the EM? It > should be cached in the EM's L1 cache, right? > > Sid Wagle wrote: > @Jonathan: Agree and Agree. I will change patch accordingly. Should we > try to address Host_Role_command and Exectuion_Command - disable caching as > well? The reason is these would goto L1 cache despite being cached explicity. > Technially all of the out getter would be served from cache unless the > commands result in contention. I believe @Noncacheable might work here, let > me know your thoughts on this. I think we should to make sure these gets are > not actual DB reads.
You're right - we should verify that retrieving the entity from the EM repeatedly doesn't hit the DB (it shouldn't). We should also probably do this for our other business objects like you mentioned. @NonCacheable shouldn't be needed here - we want it cached in the EM. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41477/#review110954 ----------------------------------------------------------- On Dec. 18, 2015, 1:25 p.m., Sid Wagle wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41477/ > ----------------------------------------------------------- > > (Updated Dec. 18, 2015, 1:25 p.m.) > > > Review request for Ambari, Jonathan Hurley, Myroslav Papirkovskyy, and Nate > Cole. > > > Bugs: AMBARI-14411 > https://issues.apache.org/jira/browse/AMBARI-14411 > > > Repository: ambari > > > Description > ------- > > *Preliminary patch* > > Symptom: > > {code} > Local Exception Stack: > Exception [EclipseLink-6004] (Eclipse Persistence Services - > 2.5.2.v20140319-9ad6abd): org.eclipse.persistence.exceptions.QueryException > Exception Description: The object > [org.apache.ambari.server.orm.entities.ClusterConfigEntity@3646b3a8], of > class [class org.apache.ambari.server.orm.entities.ClusterConfigEntity], with > identity hashcode (System.identityHashCode()) [364,075,546], > is not from this UnitOfWork object space, but the parent session's. The > object was never registered in this UnitOfWork, > but read from the parent session and related to an object registered in the > UnitOfWork. Ensure that you are correctly > registering your objects. If you are still having problems, you can use the > UnitOfWork.validateObjectSpace() method to > help debug where the error occurred. For more information, see the manual or > FAQ. > at > org.eclipse.persistence.exceptions.QueryException.backupCloneIsOriginalFromParent(QueryException.java:298) > at > org.eclipse.persistence.internal.sessions.UnitOfWorkImpl.getBackupClone(UnitOfWorkImpl.java:1995) > at > org.eclipse.persistence.internal.sessions.UnitOfWorkImpl.registerExistingObject(UnitOfWorkImpl.java:3976) > at > org.eclipse.persistence.internal.sessions.UnitOfWorkImpl.registerExistingObject(UnitOfWorkImpl.java:3894) > at > org.eclipse.persistence.mappings.CollectionMapping.buildElementUnitOfWorkClone(CollectionMapping.java:308) > at > org.eclipse.persistence.mappings.CollectionMapping.buildElementClone(CollectionMapping.java:321) > at > org.eclipse.persistence.internal.queries.ContainerPolicy.addNextValueFromIteratorInto(ContainerPolicy.java:217) > {code} > > Likely Cause: > Stale clusterEntity reference points to a detached entity which gets tried to > be merged, the Cascaded relationship throws the error on persist. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java > a0c6cfc > > Diff: https://reviews.apache.org/r/41477/diff/ > > > Testing > ------- > > Unit testing in progress. > > > Thanks, > > Sid Wagle > >
