TBH, I have no idea what happens to comments on a Pull Request when you squash and force push.
I'd just leave the multiple commits. We can squash them later. On Tue, Apr 7, 2015 at 1:59 PM, John O'Hara <joh...@redhat.com> wrote: > Steve, > > I have made changes based on the github comments and your comments below ( > https://github.com/johnaoahra80/hibernate-orm/commits/HHH-9701). > > Do you want me to squash the commits down to one? Not sure how this would > effect the comments you have already made on GH > > Thanks > > John > > > On 06/04/15 04:51, Steve Ebersole wrote: > > > > On Thu, Apr 2, 2015 at 4:45 AM, John O'Hara <joh...@redhat.com> wrote: > >> Steve, >> >> I have pushed a proposal for HHH-9701 to: >> https://github.com/johnaoahra80/hibernate-orm/tree/HHH-9701 >> >> There are a couple of areas that I would appreciate feedback; >> >> 1) Serialization/Deserialization - EntityEntries implementations can be >> serialized and each implementation provide their own serialization method. >> I have modified the serialization of EntityEntry in EntityEntryContext to >> write the Implementation class to the OutputStream so the correct class can >> be used to deserialize the object stream. Is the exception handling >> sufficient here, or do we need more robust handling of deserialization >> exceptions? : see >> https://github.com/johnaoahra80/hibernate-orm/commit/ec9b1fa3b97131ff1e65a1cc30ff6e4f2d2c8a28#diff-b55e1e51b30abe8d3c280bb22aeb6a44R380 >> > > I added some comments to that section. Also, overall I would extract > the deserialization bit into a separate method (deserializeEntityEntry). > > > > 2) In our (perf team) use case, we want to be able to share the >> ImmutableEntityEntry between sessions when they are referenced cached in >> the 2lc. I have modified EntityEntryContext to not null >> managedEntity.$$_hibernate_setEntityEntry if the EntityEntry is an instance >> of ImmutableEntityEntry. Do we need to add an extra checks here, to ensure >> that the entity is Reference Cached? I am not sure how we would test that >> case? : see >> https://github.com/johnaoahra80/hibernate-orm/commit/ec9b1fa3b97131ff1e65a1cc30ff6e4f2d2c8a28#diff-b55e1e51b30abe8d3c280bb22aeb6a44L281 >> > > It really depends on what y'all consider the trigger for using > ImmutableEntityEntry. When would the EntityPersister use the > EntityEntryFactory producing ImmutableEntityEntry instances? > a) when the entity is marked immutable? > b) when the entity is marked immutable *and* we need to cache it by > reference? > c) some other condition? > > I agree that we should only not clear that reference when the entity is > enabled for cache-by-reference. How that plays into this depends on the > answer to the above question. > > If (a), then I think that yes it makes sense to add a check to only > clear the ManagedEntity's EntityEntry reference if using cache-by-reference. > > If (b), then the EntiytPersister is only using the EntityEntryFactory > producing ImmutableEntityEntry instances when both are true. So the fact > that an entry is an instance of ImmutableEntityEntry indicates that we need > to not clear it from the ManagedEntity. > > > > >> 3) Lock Mode: Steve you mentioned about not doing locking for Immutable >> entities. Where is the locking implemented? Would it be sufficient to >> simply set the LockMode on the ImmutableEntityEntry to NONE/READ_ONLY when >> setLockMode is called? >> > > > Locking is implemented in many places. > > What I had in mind, in terms of implementation for EntityEntry, is > somewhat influenced by the choice between ignore versus exception in cases > where something is not supported. Basically I had thought to throw an > exception in ImmutableEntityEntry#setLockMode or to simply ignore the call > altogether. This is not a great solution. > > It is hard for me to justify ignoring the lock request in all cases. > What does everyone else think? > > > > > > -- > John O'harajoh...@redhat.com > > JBoss, by Red Hat > Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, > Windsor, Berkshire, SI4 1TE, United Kingdom. > Registered in UK and Wales under Company Registration No. 3798903 Directors: > Michael Cunningham (USA), Charlie Peters (USA), Matt Parsons (USA) and > Michael O'Neill (Ireland). > > > _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
