The only distinction we care about is EntityPetsister#isMutable. The
read-only case should be handled by the normal MutableEntityEntry. In
other words there is nothing to pass to EntityPersister to make a decision
based on; just getEntityEntryBuilder ().
As far as LockMode, I think it makes the most sense to not do locking for
immutable entities. Not sure if no-op or exception is the best way to
handle. But regardless either way means we do not account for locking for
the immutable - entity EntityEntry.
Which brings up another point. We should decide whether the
mutable/immutable in these names refers to the fact that the entity is
mutable/immutable or refers to the state of the EntityEntry itself. I
think it is best to agree on that semantic now before we potentially start
using these in situations they were not designed for.
On Mar 27, 2015 8:11 AM, "John O'Hara" <joh...@redhat.com> wrote:
>
>
> On 27/03/15 12:14, Sanne Grinovero wrote:
>
>> Also from OGM and Search perspective, we're not expecting any of this
>> to be backported.
>> It's not a functional blocker, "just" significant performance
>> improvements.
>>
>> How do we move further? John, I'm not familiar with the requirements
>> for the bytecode instrumentation, but can help a bit on the other
>> aspects. I could either work on extracting the patch of converting the
>> EntityEntry into an interface from your draft, or if you have time to
>> polish that I could review that PR. After that change is integrated,
>> it should be easier for us to work in parallel on different areas and
>> draft some ideas with the factory context idea.
>>
> I had started to work on extracting just the EntityEntryInterface and
> factory, so can continue on that. Which issue do we want to log this under.
> There is already HHH-9265?
>
>> +1 to an EntityPersister:EntityEntryFactory 1:1 relation, glad to see
>> it makes sense in your minds as well.
>>
>> To make a decision on the Immutable entities vs Read Only entities..
>> are we good in applying the optimisation only to Immutable entities or
>> do we need to find a way for entities which happen to be loaded
>> read-only too? I might have an idea to play with for that, but it
>> obviously gets a bit more complex.
>>
> I think for our case that we only need Immutable entities, not sure of any
> other scenarios where you would need to optimise Read Only entities
>
>>
>> @Steve, I guess we're looking at you for advice on what needs to be
>> done for the Lock state. I'm understanding that needs to be addressed
>> right?
>>
>> Thanks,
>> Sanne
>>
>>
>>
>>
>> On 27 March 2015 at 11:42, John O'Hara <joh...@redhat.com> wrote:
>>
>>> Steve,
>>>
>>> From our point of view, we would not be looking to backport any features
>>> introduced to any of the ORM4 releases. Our focus has shifted to the next
>>> major release of EAP as we have a number of issues we need to overcome.
>>> Getting this feature into ORM5 sooner rather than later is essential for
>>> our
>>> use case in the future.
>>>
>>> Thanks
>>>
>>> John
>>>
>>>
>>> On 27/03/15 11:36, Steve Ebersole wrote:
>>>
>>> BTW, if we go this route this *cannot* be backported because we do allow
>>> users to override EntityPersister; its an extension point. Adding a new
>>> method to extension points in bugfixes is a no-no.
>>>
>>> On Fri, Mar 27, 2015 at 6:35 AM, Steve Ebersole <st...@hibernate.org>
>>> wrote:
>>>
>>>> An EntityPersister describes each entity you defined in your model. Its
>>>> one-to-one between @Entity and EntityPersister. And really, it is
>>>> totally
>>>> reasonable to have the EntityPersister determine the "EntityEntry
>>>> builder"
>>>> to use. That also eliminates needing to develop
>>>> yet-another-extension-point
>>>> because if we expose it from EntityPersister OGM already swaps
>>>> EntityPersister impls so this would JustWork.
>>>>
>>>> On Fri, Mar 27, 2015 at 6:04 AM, John O'Hara <joh...@redhat.com> wrote:
>>>>
>>>>> On 26/03/15 16:22, Steve Ebersole wrote:
>>>>>
>>>>> The contract I suggested btw is missing one use case I know we have
>>>>> discussed in regards to OGM a few times... the ability to store
>>>>> references
>>>>> to datastore specific locators (I think the use case was to
>>>>> efficiently load
>>>>> collections through that datastore reference). But that (obviously)
>>>>> requires passing extra, specific state into the createEntityEntry
>>>>> method.
>>>>> Not really sure the best way to handle such things tbh.
>>>>>
>>>>> If we did have a "creation Context" parameter passed to the
>>>>> createEntityEntry, we would be instantiating more objects, however if
>>>>> we
>>>>> went with Sanne' suggestion if caching the EnityEntryFactory with the
>>>>> EntityPersister, how many objects would be actually created? I am not
>>>>> sure
>>>>> about the relationship between Entity, EntityEntry and Session but
>>>>> could
>>>>> this a possibility if there is a 1-1 relationship between
>>>>> EntityPersister
>>>>> and no. of different Entities definitions?
>>>>>
>>>>>
>>>>> On Thu, Mar 26, 2015 at 11:16 AM, Steve Ebersole <st...@hibernate.org>
>>>>> wrote:
>>>>>
>>>>>> Apparently I hit some key combo that means send :) To finish up...
>>>>>>
>>>>>> On Thu, Mar 26, 2015 at 10:58 AM, Steve Ebersole <st...@hibernate.org
>>>>>> >
>>>>>> wrote:
>>>>>>
>>>>>>>
>>>>>>> On Thu, Mar 26, 2015 at 10:20 AM, Sanne Grinovero <
>>>>>>> sa...@hibernate.org>
>>>>>>> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> Concurrency
>>>>>>>> Since the goal is to share the ImmutableEntityEntry instance
>>>>>>>> among
>>>>>>>> multiple threads reading it, I'd rather make sure that it is really
>>>>>>>> immutable. For example it now holds onto potentially lazy
>>>>>>>> initialized
>>>>>>>> fields, such as getEntityKey().
>>>>>>>> If it's not possible to make it really immutable (all final fields),
>>>>>>>> we'll need to make it threadsafe and question the name I'm
>>>>>>>> proposing.
>>>>>>>>
>>>>>>>
>>>>>>> The specific use-case John is interested in does indeed need to be
>>>>>>> completely thread-safe and fully concurrent.
>>>>>>>
>>>>>>> LockMode
>>>>>>>> From a logical perspective of users, one might think that an
>>>>>>>> entity
>>>>>>>> being "immutable" doesn't necessarily imply I can't lock on it..
>>>>>>>> right? I'm not sure how far it could be a valid use case, but these
>>>>>>>> patches are implying that one can't lock an immutable entity
>>>>>>>> anymore,
>>>>>>>> as the lock state would be as immutable as anything else on the
>>>>>>>> EntityEntry.
>>>>>>>> Are we good with that? Alternatively one might need to think to
>>>>>>>> separate the lock state handling from the EntityEntry.
>>>>>>>>
>>>>>>>
>>>>>>> Conceptually there is nothing wrong with requesting a READ lock on an
>>>>>>> immutable entity. But like you said, what is the logical
>>>>>>> expectation there?
>>>>>>> IMO there should be none. But if we decide that it is OK to req
>>>>>>>
>>>>>>
>>>>>> But if we decide that it is OK to request a lock on an immutable
>>>>>> entity,
>>>>>> that is problematic for the idea of a "SharedEntityEntry" or an
>>>>>> "ImmutableEntityEntry" because a lock is associated with a Session
>>>>>> which is
>>>>>> what the EntityEntry is meant to model... an entity's information in
>>>>>> relation to a Session. Aka, it now needs to hold state.
>>>>>>
>>>>>>
>>>>>> Notice I said immutable here and not READ_ONLY which is a specific
>>>>>>> distinction which is important to other parts of your email that I
>>>>>>> will
>>>>>>> address in a second email.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> On smaller details:
>>>>>>>> # org.hibernate.engine.internal.SharedEntityEntry is hosted in an
>>>>>>>> .internal package, I don't think it's right to refactor all the
>>>>>>>> public
>>>>>>>> API javadoc which was referring to EntityEntry to now refer to the
>>>>>>>> internal implementation.
>>>>>>>> # things like EntityEntryExtraState should probably get moved to
>>>>>>>> .internal packages as well now - we couldn't do that before without
>>>>>>>> breaking either encapsulation or APIs.
>>>>>>>>
>>>>>>>
>>>>>> +1
>>>>>>
>>>>>>
>>>>>>>> In terms of git patches, the complexity of the changeset risks to
>>>>>>>> get
>>>>>>>> a bit our of hand. What about we focus on creating a clean pull
>>>>>>>> request which focuses exclusively on making EntityEntry an
>>>>>>>> interface,
>>>>>>>> and move things to the right packages and javadoc?
>>>>>>>>
>>>>>>>
>>>>>> Agree 100%
>>>>>>
>>>>>> You'd have a trivial EntitEntryFactory, and we can then build the
>>>>>>>> evolution on top of that, not least maybe helping out by challenging
>>>>>>>> some points in parallel work.
>>>>>>>> These are the things I'd leave for a second iteration:
>>>>>>>> - add various implementations of EntityEntry iteratively, as
>>>>>>>> needed
>>>>>>>> - the strategy such a Factory would use the pick an implementation
>>>>>>>> - ultimately, make it possible for an integrator to override such
>>>>>>>> a
>>>>>>>> Factory
>>>>>>>>
>>>>>>>
>>>>>> This all seems reasonable. For 5.0 I think the most important
>>>>>> thing is
>>>>>> to nail down the idea of EntityEntry as an interface, introduce a
>>>>>> Factory
>>>>>> for building them and agree on the signature for building them.
>>>>>> Granted we
>>>>>> may need iteration to finalize the actual Factory signature,
>>>>>> especially as
>>>>>> OGM finally starts to use it, but I think that in general we can get
>>>>>> it
>>>>>> pretty close. Worst case we just pass high-level constructs like the
>>>>>> EntityPersister (which OGM supplies custom impls) and the Session and
>>>>>> all
>>>>>> the "EntityEntry state". For the purpose of starting a discussion:
>>>>>>
>>>>>> public interface EntityEntryFactory {
>>>>>> public EntityEntry createEntityEntry(
>>>>>> PersistenceContext persistenceContext,
>>>>>> EntityPersister persister,
>>>>>> Status status,
>>>>>> Serializable id,
>>>>>> Object version,
>>>>>> Object[] loadedState,
>>>>>> Object rowId,
>>>>>> LockMode lockMode,
>>>>>> boolean existsInDatabase,
>>>>>> boolean disableVersionIncrement,
>>>>>> boolean lazyPropertiesAreUnfetched);
>>>>>> }
>>>>>>
>>>>>> I would suggest a "creation context" method param object to pass in
>>>>>> here, but seeing as how we are trying to stop instantiations...
>>>>>>
>>>>>> I would prefer the definition of the EntityEntryFactory to use be
>>>>>> defined via SessionFactoryBuilder interface. I already have plans to
>>>>>> have a
>>>>>> auto-loaded hook for integrators to be able to adjust the
>>>>>> SessionFactoryBuilder. But as a Service is fine too.
>>>>>>
>>>>>> For example with Hibernate OGM we might want to override / re
>>>>>>>> configure the factories to use custom EntityEntry implementations -
>>>>>>>> requirements are not fully clear at this point but it seems likely.
>>>>>>>>
>>>>>>>> The priority being to define the API as that would be a blocker for
>>>>>>>> 5.0, we have then better choices to leave more smarter and advanced
>>>>>>>> EntityEntry implementations for the future; we'd still need to
>>>>>>>> implement at least the essential ones to make sure the API of the
>>>>>>>> EntityEntryFactory has all the context it needs.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Sanne
>>>>>>>>
>>>>>>>>
>>>>>>>> On 24 March 2015 at 09:27, John O'Hara <joh...@redhat.com> wrote:
>>>>>>>>
>>>>>>>>> Steve,
>>>>>>>>>
>>>>>>>>> Have you had chance to look at this? Do you have any
>>>>>>>>> comments/observations?
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>>
>>>>>>>>> John
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 17/03/15 09:24, John O'Hara wrote:
>>>>>>>>>
>>>>>>>>> Steve,
>>>>>>>>>
>>>>>>>>> I have been having a think about the EntityEntry interface, and
>>>>>>>>> have
>>>>>>>>> forked
>>>>>>>>> a branch here:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> https://github.com/johnaoahra80/hibernate-orm/
>>>>>>>>> tree/EntityEntryInterface
>>>>>>>>>
>>>>>>>>> I know it is nowhere near complete, but was this the sort of idea
>>>>>>>>> you had in
>>>>>>>>> mind?
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>>
>>>>>>>>> John
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 13/03/15 09:44, John O'Hara wrote:
>>>>>>>>>
>>>>>>>>> EntityEntry retains a reference to a persistenceContext internally
>>>>>>>>> that
>>>>>>>>> org.hibernate.engine.spi.EntityEntry#setReadOnly makes calls to,
>>>>>>>>> is
>>>>>>>>> this
>>>>>>>>> where the session reference is kept? As
>>>>>>>>> org.hibernate.engine.spi.PersistenceConext is an interface could
>>>>>>>>> we
>>>>>>>>> have a
>>>>>>>>> different implementation for this use case? e.g. an
>>>>>>>>> ImmutablePersistenceContext that could be shared across sessions?
>>>>>>>>>
>>>>>>>>> For the bytecode enhancement, could we change the enhancer so that
>>>>>>>>> it adds
>>>>>>>>> an EntityEntry interface with javassist.
>>>>>>>>> ClassPool.javassist.ClassPool.makeInterface()() as opposed to
>>>>>>>>> adding
>>>>>>>>> a class
>>>>>>>>> javassist.ClassPool.makeClass()? I need to have a look at javassit
>>>>>>>>> to
>>>>>>>>> confirm what javassist.ClassPool.makeInterface() does.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>>
>>>>>>>>> John
>>>>>>>>>
>>>>>>>>> On 12/03/15 18:52, Steve Ebersole wrote:
>>>>>>>>>
>>>>>>>>> It is possible. Although some of the changes are particularly
>>>>>>>>> painful.
>>>>>>>>> Most of EntityEntry, if it is an interface, can be made to work
>>>>>>>>> with
>>>>>>>>> your
>>>>>>>>> use case. org.hibernate.engine.spi.EntityEntry#setReadOnly I
>>>>>>>>> think
>>>>>>>>> is the
>>>>>>>>> one exception, because:
>>>>>>>>> 1) your use case needs it
>>>>>>>>> 2) it expects the Session to be available internally (its not
>>>>>>>>> passed)
>>>>>>>>>
>>>>>>>>> The bigger thing I am worried about for you is the bytecode stuff,
>>>>>>>>> as that
>>>>>>>>> ties very tightly with EntityEntry.
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>> hibernate-dev mailing list
>>>>>>>> hibernate-dev@lists.jboss.org
>>>>>>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>> --
>>>>> John O'Hara
>>>>> joh...@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).
>>>>>
>>>>>
>>>
>>> --
>>> John O'Hara
>>> joh...@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).
>>>
>>>
>
> --
> John O'Hara
> joh...@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
