[hibernate-dev] HHH-13916 / WFLY-13259

Fri Apr 17 16:34:30 EDT 2020

On Thu, 16 Apr 2020 at 01:06, Gail Badner <gbadner at redhat.com> wrote:
>
> Hi Galder,
>
> Thanks for the response. I agree this would be a good change for H6. I'm happy to hear that this is on someone's radar for Infinispan.
>
> Sanne, regarding:
> > I'd say if you clearly mark the old method deprecated (or even remove it?) it
> will be guaranteed we don't forget about this improvement.
>
> Are you suggesting that SharedSessionContractImplementor#getSessionIdentifier be deprecated?
> If so, it has other uses, so it really can't be deprecated.

Right, sorry let me clarify that. What I'm getting at is that clearly
the current method is being used in different contexts; I see at leat
two main ones:
 - requiring a Serializable identifier (across the wire for
clustering, and to be stored in databases) - typically an UUID.
 - some form of "identity token" as we discussed on Zulip, which is
far cheaper to generate but unsuitable for serialization.

When we get to such realizations, I generally think it's a good idea
to create more suitably named versions for *both* such use cases so to
avoid the overloaded notion we currently have.
Then, as second step deprecate (or remove) the original method.

Since we're discussing a major release, I'd remove the existing
method, or if you prefer the more conservative approach create JIRA
for it to be removed just before Final if that's more convenient -
for example keeping it a little longer could facilitate testing of the
previews without already needing the custom 2LC release but
I suspect it's not really worth it to be overzealous.

Thanks,
Sanne

>
> I've moved the fix version for HHH-13916 to 6.0.0.Beta1 and created a wip/6.0 PR: https://github.com/hibernate/hibernate-orm/pull/3341 .
>
> Regards,
> Gail
>
>
> On Wed, Apr 15, 2020 at 7:58 AM Galder Zamarreno <galder at redhat.com> wrote:
>>
>> Given that there's a workaround for the issue, I'd agree with Sanne to make this an ORM 6+ only improvement.
>>
>> Indeed I'm no longer in the team and hence could not be the maintainer for such a module, but I'd be happy to discuss improvements for ORM6 caching. I've not been aware of any discussions on that yet.
>>
>> On the Infinispan side, just a couple of weeks back Tristan was asking me about the need for a remote Infinispan cache provider for ORM. Maybe ORM6 offers a good opportunity to rework the provider and make it easy to maintain a local/remote provider.
>>
>> Galder
>>
>>
>> On Wed, Apr 15, 2020 at 1:42 PM Sanne Grinovero <sanne at hibernate.org> wrote:
>>>
>>> On Wed, 15 Apr 2020 at 02:16, Gail Badner <gbadner at redhat.com> wrote:
>>> >
>>> > FWIW, there's no point in fixing HHH-13916 unless we hear that Infinispan is going to use the fix.
>>>
>>> I suppose we can expect that that will eventually happen - I just
>>> don't know when and were to best make a note of such a need? I'd say
>>> if you clearly mark the old method deprecated (or even remove it?) it
>>> will be guaranteed we don't forget about this improvement.
>>>
>>> As far as I know, there isn't an Infinispan 2LC codebase for ORM6 yet,
>>> and there will be more differences likely?
>>>
>>> Also keep in mind that Galder no longer works on Infinispan, he's now
>>> focused on GraalVM and JDK engineering. I'm glad he's still around and
>>> we might be able to bother him for suggestions and wisdom, but I'm not
>>> sure yet who's going to be responsible to create such a new module.
>>> Radim is on the performance team; as always, would be awesome to have
>>> his help but I don't know of the performance team will be able to own
>>> the module maintenance.
>>>
>>> Thanks,
>>> Sanne
>>>
>>>
>>> >
>>> > Radim/Galder, WDYT?
>>> >
>>> > Thanks,
>>> > Gail
>>> >
>>> > On Tue, Apr 14, 2020 at 3:25 PM Gail Badner <gbadner at redhat.com> wrote:
>>> >>
>>> >> Hi Sanne,
>>> >>
>>> >> I've reworked HHH-13916 to use this alternative, and set the fix version to 6-wishlist.
>>> >>
>>> >> Regards,
>>> >> Gail
>>> >>
>>> >> On Tue, Apr 14, 2020 at 1:23 AM Sanne Grinovero <sanne at hibernate.org> wrote:
>>> >>>
>>> >>> Hi Gail,
>>> >>>
>>> >>> I would go ahead with this improvement for ORM 6 and avoid spending
>>> >>> your precious time on a v5 improvement - especially if it's going to
>>> >>> require coordination with both the Infinispan and WildFly teams.
>>> >>>
>>> >>> Thanks
>>> >>>
>>> >>> On Fri, 10 Apr 2020 at 00:56, Gail Badner <gbadner at redhat.com> wrote:
>>> >>> >
>>> >>> > I've been looking into how to fix this issue:
>>> >>> >
>>> >>> > https://hibernate.atlassian.net/browse/HHH-13916
>>> >>> > https://issues.redhat.com/browse/WFLY-13259
>>> >>> >
>>> >>> > The crux of the matter is when Hibernate calls CacheHelper.fromSharedCache(
>>> >>> > session, cacheKey, cachAccess ), and the entity is not found in the cache,
>>> >>> > Infinispan stores a PendingPut containing a
>>> >>> > SharedSessionContractImplementor instance.
>>> >>> >
>>> >>> > IIUC, as an optimization, Infinispan assumes that the entity not found in
>>> >>> > the cache will ultimately be added to the cache after it is loaded from the
>>> >>> > database. If that doesn't happen, the PendingPut will expire and will
>>> >>> > eventually be removed. Until it expires,
>>> >>> > the SharedSessionContractImplementor instance cannot be garbage-collected.
>>> >>> >
>>> >>> > This is particularly a problem if the cache is not disabled while a large
>>> >>> > amount of cacheable data is being imported. This is the particular use case
>>> >>> > described by WFLY-13259. There is a reproducer attached that
>>> >>> > throws OutOfMemoryError.
>>> >>> >
>>> >>> > The obvious workaround is to set org.hibernate.CacheMode.IGNORE (or
>>> >>> > possibly CacheMode.PUT?) while importing data.
>>> >>> >
>>> >>> > I discussed this briefly with Sanne, and we agree that an improvement would
>>> >>> > be to not store a SharedSessionContractImplementor in a PendingPut at all.
>>> >>> >
>>> >>> > There is already a way to get a UUID for the session by calling
>>> >>> > SharedSessionContractImplementor#getSessionIdentifier(). Unfortunately, the
>>> >>> > implementation in AbstractSharedSessionContract indicates that frequent
>>> >>> > "UUID generations will cause a significant amount of contention".
>>> >>> >
>>> >>> > Sanne has suggested returning a "token" that is just a new Object. I've
>>> >>> > created a branch
>>> >>> > <https://github.com/gbadner/hibernate-core/tree/HHH-13916_token> [1] that
>>> >>> > does this.
>>> >>> >
>>> >>> > Infinispan would need to be updated so that PendingPut#owner is set
>>> >>> > to SharedSessionContractImplementor#getSessionToken() (instead of
>>> >>> > the SharedSessionContractImplementor object).
>>> >>> >
>>> >>> > Looking at the Infinispan code, I see that code that would be affected is
>>> >>> > in
>>> >>> > org.infinispan.hibernate.cache.commons.access.PutFromLoadValidator, which
>>> >>> > is used by infinispan-hibernate-cache-v51.
>>> >>> >
>>> >>> > IIUC, in order to fix this any time soon for WildFly or EAP 7.x, [1] would
>>> >>> > have to be backported to both Hibernate ORM 5.1 and 5.3 branches, and the
>>> >>> > Hibernate versions would have to be updated in Infinispan before Infinispan
>>> >>> > could be updated to use SharedSessionContractImplementor#getSessionToken().
>>> >>> >
>>> >>> > Galder/Radim, are there any plans for
>>> >>> > dropping infinispan-hibernate-cache-v51?
>>> >>> >
>>> >>> > Are there other places where the SharedSessionContractImplementor is stored
>>> >>> > in the cache?
>>> >>> >
>>> >>> > Aside from infinispan-hibernate-cache-v51, do you see anything about [1]
>>> >>> > that would cause problems?
>>> >>> >
>>> >>> > If not, when do you think we could coordinate this change? Do we need to
>>> >>> > wait for Hibernate ORM 6.0?
>>> >>> >
>>> >>> > This is considered an improvement, so it's not urgent. It would be nice to
>>> >>> > fix this though.
>>> >>> >
>>> >>> > Galder/Radim, please provide your input so we figure out when it can be
>>> >>> > fixed.
>>> >>> >
>>> >>> > Thanks,
>>> >>> > Gail
>>> >>> >
>>> >>> > [1] https://github.com/gbadner/hibernate-core/tree/HHH-13916_token
>>> >>> > [2]
>>> >>> > _______________________________________________
>>> >>> > hibernate-dev mailing list
>>> >>> > hibernate-dev at lists.jboss.org
>>> >>> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>> >>> >
>>> >>>
>>>