ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Hurley" <jhur...@hortonworks.com>
Subject Re: Review Request 41477: Fix stale cluster entity reference which results in merge issues
Date Mon, 21 Dec 2015 17:14:01 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message