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

Steve Ebersole steve at hibernate.org
Fri Apr 17 16:54:07 EDT 2020 

Not really sure what "better" names you have in mind, but to me only one of
these 2 is really an identifier.  This new method is more a niche value to
me.  So not sure why you want to rename the original.

On Fri, Apr 17, 2020 at 3:36 PM Sanne Grinovero <sanne at hibernate.org> wrote:

> 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
> >>> >>> >
> >>> >>>
> >>>
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
>

More information about the hibernate-dev mailing list