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 <sa...@hibernate.org> wrote: > On Thu, 16 Apr 2020 at 01:06, Gail Badner <gbad...@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 <gal...@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 <sa...@hibernate.org> > wrote: > >>> > >>> On Wed, 15 Apr 2020 at 02:16, Gail Badner <gbad...@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 <gbad...@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 < > sa...@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 <gbad...@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@lists.jboss.org > >>> >>> > https://lists.jboss.org/mailman/listinfo/hibernate-dev > >>> >>> > > >>> >>> > >>> > _______________________________________________ > hibernate-dev mailing list > hibernate-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev > > _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev