[17:16] <sannegrinovero> sebersole: I can spend some time to aim at HHH-9780 this weekend, but will need some pointers from you
[17:17] <sebersole> sannegrinovero: tbh i dont really know what you mean with your part of that :)
[17:17] <sebersole> also that seems directly at odds with the idea of leveraging the cache impls notion of a cache key
[17:18] <sebersole> bascially, there are 3 concepts here
[17:18] <sebersole> and i just dont see how we can model them all as the same class
[17:19] <sannegrinovero> maybe not, I just need to understand it better. You mentioned the 2lc implementors create their own, I didn't find that
[17:19] <sebersole> especially as delegated to the cache impl
[17:19] <sannegrinovero> you have a pointer?
[17:19] <sebersole> actually its something you mentioned :)
[17:19] <sebersole> so you tell me...
[17:20] <sebersole> lets take infinispan e.g...
[17:20] <sannegrinovero> yes I also remember that :) I'm not finding it anymore though, wondering if that's just an outdated obeservation.
[17:20] <sebersole> ultimately I ask inifispan to store some data for me
[17:20] <sebersole> i pass it (1) the data to cache
[17:20] <sebersole> and
[17:20] <sebersole> (2) my CacheKey
[17:21] <sebersole> what does infinispan do there internally>
[17:21] <sebersole> ?
[17:21] <sebersole> does it directly key the entry using my CacheKey?
[17:21] <sebersole> surely not right?
[17:21] <sannegrinovero> looking at master today, it seems to just use that key instance w/o rewraps
[17:21] <sannegrinovero> (which is not what I remember)
[17:21] <sebersole> ok then
[17:21] <sebersole> well...
[17:22] <sebersole> master of what>
[17:22] <sebersole> hibernate-infinispan?
[17:22] <sebersole> or
[17:22] <sebersole> infinispan?
[17:22] <sannegrinovero> it also feels wrong actually because it means it can hardly optimize it for its custom serialization strategies
[17:22] <sannegrinovero> hibernate-infinispan
[17:22] <sebersole> well more i mean what infnispan does after hibernate-infinispan hands it the info
[17:22] <sannegrinovero> so sebersole, proposal:
[17:23] <sebersole> i thought that was the issue
[17:23] <sannegrinovero> 1# we change the CacheKey to be an interface
[17:23] <sebersole> ok...
[17:23] <sannegrinovero> 2# and then add a "createCacheKey" method on the 2lc provider SPI
[17:23] <sannegrinovero> so to allow the providers to factor custom keys as needed
[17:23] <sebersole> sure, thats what i suggested :)
[17:23] <sannegrinovero> (any custom optimisation there can wait)
[17:23] <sannegrinovero> ok great
[17:24] <sebersole> but
[17:24] <sebersole> we dont even need CacheKey interface
[17:24] <sebersole> we never use those data values
[17:24] <sannegrinovero> that's right the 2lc interface accepts "Object" for keys
[17:24] <sebersole> really createCacheKey can return Object
[17:24] <sebersole> right
[17:24] <sebersole> i mean
[17:25] <sebersole> might be a nice chance to improve that
[17:25] <sannegrinovero> but should we keep that? It seems nicer to change the SPI interface to only accept the typesafe CacheKey
[17:25] <sebersole> 2 things i hate passing
[17:25] <sebersole> Object
[17:25] <sebersole> String
[17:25] <sannegrinovero> +1
[17:25] <sebersole> i am fine with that
[17:26] <sannegrinovero> I'd assume that once we offload the factory responsibility to the cache implementor, they should be able to make keys conforming to it w/o drawbacks.. right?
[17:26] <sebersole> alex may not be :)
[17:26] <sannegrinovero> I'll make this change as PR and then ask for feedback from Alex and Galder
[17:26] <sebersole> sounds perfect
[17:27] <sannegrinovero> ok, second part of the problem:
[17:27] <sebersole> but back to
[17:27] <sebersole> rightr
[17:27] <sebersole> and so here is one problem
[17:27] <sannegrinovero> the key to EntityEntry is a different beast entirely then, right?
[17:27] <sebersole> we removed the idea of some of the selectors from EntityKey
[17:28] <sebersole> well i assume you mean EntityKey, notEntityEntry
[17:28] <sannegrinovero> yes, EntityKey, the key to EntityEntry ;)
[17:29] <sebersole> so second level cache needs to segment things differently then PC
[17:29] <sebersole> it needs to account for tenancy e.g.
[17:29] <sebersole> whereas the PC does not
[17:29] <sebersole> (the PC is inherently tied to *a* tenant)
[17:30] <sannegrinovero> good point
[17:31] <sannegrinovero> but what if we were to add - again on the 2lc SPI - a method like "CacheKey convert(EntityKey, additional metadata like tenantId )
[17:31] <sannegrinovero> or vice-versa could be even more interesting
[17:31] <sannegrinovero> would there be some method in ORM to benefit from invoking this conversion?
[17:32] <sebersole> well it assumes having the EntityKey
[17:32] <sannegrinovero> (some strategic method)
[17:32] <sebersole> or creating it if we dont
[17:32] <sebersole> its this later point that is concerning
[17:32] <sannegrinovero> well if you don't, you'd invoke the other method we agreed on, the factory.
[17:32] <sebersole> oh
[17:32] <sebersole> you mean having multiple methods
[17:33] <sebersole> sure
[17:33] <sannegrinovero> yes, to add a conversion method as something on top of the factory
[17:33] <sannegrinovero> see there are for sure cases in which one implementation could serve fine for both use cases (say multi-tenancy is disabled, for one)
[17:34] <sebersole> well depending on the declaration of CacheKey... the "other way" may not be needed
[17:34] <sannegrinovero> what I don't know, if there are points in code in ORM in which you'd have one and need the other
[17:35] <sebersole> ohhhhh
[17:35] <sebersole> another oddity
[17:35] <sebersole> so CacheKey can be used to key entity data
[17:35] <sebersole> but
[17:35] <sebersole> it can also be used to cache collection data
[17:36] <sebersole> so its related to org.hibernate.engine.spi.CollectionKey as well
[17:36] <sebersole> CollectionKey is the EntityKey corollary in the PC
[17:36] <sannegrinovero> ah, contract-wise that gets ugly unless we seprate the notion of cache key into two different types too
[17:37] <sebersole> thats why its called "entityOrRoleName" in CacheKey
[17:37] <sebersole> which we could
[17:37] <sebersole> since the Regions are distinct
[17:37] <sebersole> CollectionRegion/CollectionRegionAccessStrategy
[17:37] <sebersole> versus
[17:37] <sannegrinovero> considering they're currently accepting "Object" I guess it won't be too bad ;)
[17:37] <sebersole> EntityRegion/EntityRegionAccessStrategy
[17:38] <sebersole> we might have to parametize the access stratregy though for that to work
[17:38] <sebersole> as I think they share get() etc methods
[17:38] <sebersole> yeah... org.hibernate.cache.spi.access.RegionAccessStrategy
[17:39] <sebersole> we'd need RegionAccessStrategy<T> - where <T> is "key type"
[17:39] <sannegrinovero> so each access strategy already is a different contract.. they could then accept the same type CacheKey but convert from/to the CollectionKey or EntityKey ?
[17:40] <sebersole> i think having the caches convert *to* CacheKey makes sense
[17:40] <sebersole> i actually dont think the other way makes sense
[17:40] <sebersole> imo
[17:40] <sebersole> again, assuming CacheKey exposes its state
[17:40] <sebersole> at least the ones we care about :)
[17:41] <sannegrinovero> ok, the biggest win is definitely from cache to EntityKey (as one would hope there are more cache hits than cache stores)
[17:41] <sannegrinovero> so my primary goal is to see if we can avoid allocating an EntityKey after a cache hit
[17:42] <sebersole> are you thinking to wrapo the EntityKey?
[17:42] <sebersole> if so...
[17:42] <sebersole> then here is what i think makes sense...
[17:42] <sebersole> 1) split CacheKey into 2 interfaces
[17:42] <sebersole> EntityCacheKey
[17:42] <sebersole> CollectionCacheKey
[17:43] <sebersole> they each define just one method
[17:43] <sebersole> EntityCacheKey#toEntityKey
[17:43] <sebersole> CollectionCacheKey#toCollectionKey
[17:43] <sannegrinovero> ah, sweet
[17:43] <sannegrinovero> then you could have some such implementations just return "this", right?
[17:43] <sebersole> 2) RegionFactory provides the factory for creating these cache keys
[17:44] <sebersole> right
[17:44] <sebersole> this factory would allow:
[17:44] <sebersole> a) creation from simple values
[17:44] <sebersole> b) creation from EntityKey/CollectionKey +
[17:45] <sannegrinovero> nice, that all feels fitting well the needs
[17:45] <sebersole> our spi can provide a Helper for these
[17:45] <sannegrinovero> I'm only wondering if there is a practical use for 2)b
[17:45] <sannegrinovero> you'd need to point me to the ORM code which could benefit from that, I don't have metrics for this case
[17:46] <sebersole> it depends what you envision practicallty inside these cache key impls
[17:46] <sannegrinovero> although, this could be done after an API freeze.. just making sure there is a practical use for it.
[17:46] <sebersole> are they holding (wrapping) EntityKey/CollectionKey instances?
[17:46] <sebersole> its mainly the after action stuff
[17:47] <sebersole> so we are pushing flushed changes to the cache
[17:47] <sebersole> we'd have the PC keys
[17:48] <sannegrinovero> at best the 2lc could reuse the same instance, but the key type would need to satisfy the cache key contract too
[17:48] <sannegrinovero> the alternative is a wrap but I wonder if that would still win us something
[17:49] <sebersole> not following
[17:49] <sannegrinovero> I mean, when it comes to store things in the cache, and you have the PC keys
[17:49] <sebersole> right, which is why isaid:
[17:50] <sannegrinovero> *ideally* one would like to reuse the PC keys as-is (if configuration allows - like again no multi-tenant)
[17:50] <sebersole> [10:46] <sebersole> it depends what you envision practicallty inside these cache key impls
[17:50] <sannegrinovero> yes
[17:50] <sebersole> but imagine:
[17:51] <sebersole> EntityCacheKeyImpl implements EntityCacheKey { private final EntityKey entityKey; private final tenantId; ... }
[17:51] <sebersole> so its ^^ toEntityKey is simple
[17:51] <sebersole> and saves some instantiations
[17:51] <sannegrinovero> right that's the stronger benefit
[17:52] <sannegrinovero> but I was now looking at the possibility to also save some allocation in the inverse transformation
[17:52] <sebersole> wdym?
[17:52] <sannegrinovero> which is a minor, more questionable win
[17:52] <sebersole> EntityKey->EntityCacheKey?
[17:52] <sannegrinovero> yes
[17:53] <sannegrinovero> to be able to use the EntityKey instance as key within the caches.
[17:53] <sebersole> well i think we already track that
[17:53] <sannegrinovero> track?
[17:54] <sebersole> well that could happen, sure, but only if the other "selectors" (tenantId, etc) are the same across sessions
[17:54] <sannegrinovero> yes
[17:54] <sannegrinovero> it's not something we can enforce, especially not as a type because of those reasons
[17:54] <sebersole> yeah, i thought EntityEntry cached the cache key
[17:54] <sannegrinovero> but the key factory or a smart cache implementor could take advantage from some configurations
[17:55] <sebersole> well as far as tenancy goes...
[17:55] <sebersole> you dont really need a config
[17:56] <sebersole> if the tenantId is ever null you know the SF is not using multi-trenancy
[17:56] <sannegrinovero> right, I just mean that conceptually the implementor could be able to do some smart choices in some circumstances - even if not all.
[17:56] <sebersole> and the EntityKey *shoul d be* safe to use (more or less) as the cache key
[17:57] <sebersole> but of course, with this new split in EntityEntry that is much more difficult mnow
[17:57] <sannegrinovero> it gets a bit tricky as those keys will need to implement a safe equals contract
[17:57] <sannegrinovero> if potentially multiple different implementations of those keys are in the same cache
[17:57] <sebersole> yeah, i see why you'd want to use EntityKey as the cache key, but...
[17:58] <sebersole> i just dont think its feasible
[17:58] <sebersole> doing so would put a lot of stress on the PC to understand this too
[17:58] <sebersole> "hey can my EntityKeys be used as a cache key?  if so, I need to build these special ones..."
[17:58] <sannegrinovero> it's complex if we don't do a further step. but what if all EntityKey were created by that same key factory?
[17:59] <sebersole> i think thats wrong too
[17:59] <sebersole> why should the l2 cache be involved in building PC keys?
[17:59] <sebersole> again perf wise i get it
[17:59] <sannegrinovero> ok, if it doesn't fit the concept then let's stop at the plan above
[17:59] <sebersole> conceptually, design-wise.... it just does nt fit
[18:00] <sannegrinovero> design-wise, I see it as we need a unique way to identify an entity, and you allow the 2nd level cache implementor to set the factory globally.
[18:00] <sebersole> well
[18:01] <sebersole> "unique way to identify an entity" is a big gloss  over :)
[18:01] <sebersole> thats the problem
[18:01] <sannegrinovero> well, per persister ;)
[18:01] <sebersole> its not even per persister
[18:01] <sebersole> well, i guess if you include tenant it in that sure
[18:02] <sebersole> do remember though that *you* were the one that had me remove tenantId from EntityKey ;)
[18:02] <sannegrinovero> yes I remember :)
[18:02] <sebersole> otherwise this is not a discussion :)
[18:03] <sebersole> but i do like the rest of the design we scioped out
[18:03] <sebersole> feels right
[18:03] <sannegrinovero> but ok the plan you described above seems very sound, up to the 2/b part which is of lower value
[18:04] <sebersole> sannegrinovero: i dont think it is :)
[18:04] <sebersole> really its just an overloaded method form
[18:05] <sannegrinovero> no? I thought we had just decided that this last part gets too complex, for low benefits
[18:05] <sebersole> well you were talking about actually *using* the EntityKey and the EntityCacheKey
[18:05] <sebersole> thats different
[18:05] <sebersole> 2.a is this:
[18:07] <sebersole> EntityCacheKey cacheKey = regionFactory().getKeyFactory().createEntityCacheKey( entityName, key, tenantId, .. )
[18:07] <sebersole> 2.b is simply an overload:
[18:07] <sebersole> EntityCacheKey cacheKey = regionFactory().getKeyFactory().createEntityCacheKey( theEntityKey, tenantId, .. )
[18:08] <sannegrinovero> ok we can add it as convenience, but we agree that it's likely the implementor will need to allocate a new EntityCacheKey, right?
[18:09] <sannegrinovero> instance reuse seems out of reach
[18:09] <sebersole> well, not necessarily
[18:10] <sebersole> but thats not my primary design goal
[18:10] <sebersole> but
[18:10] <sebersole> look at it...
[18:10] <sebersole> regionFactory().getKeyFactory().createEntityCacheKey( theEntityKey, tenantId, .. )
[18:10] <sebersole> and keep a few things in mind...
[18:11] <sebersole> 1) the *sole* contract fior EntityCacheKey is #toEntityKey
[18:11] <sannegrinovero> right, it looks sexy :) And it doesn't hurt to allow the implementor to do something smarter eventually than what we've thought of today.
[18:11] <sebersole> so its actually possible for the EntityKey to implement that contract too
[18:11] <sebersole> with a caveat
[18:12] <sebersole> "hey... EntityKey *can* act as a EntityCacheKey, but only in non-multi-tenant environments"
[18:12] <sebersole> and createEntityCacheKey knows that
[18:12] <sebersole> ergo...
[18:12] <sebersole> :)
[18:13] <sannegrinovero> but only if EntityKey implements EntityCacheKey, for typesafety
[18:14] <sannegrinovero> I mean, to have the method compile..
[18:14] <sebersole> @Override public EntityCacheKey createEntityCacheKey(EntityKey theEntityKey, String tenantId)) { return tenantId == null ? theEntityKey : new EntityCacheKeyImpl( theEntityKey, tenantId ); }
[18:14] <sebersole> [11:11] <sebersole> 1) the *sole* contract fior EntityCacheKey is #toEntityKey
[18:14] <sebersole> [11:11] <sebersole> so its actually possible for the EntityKey to implement that contract too
[18:14] <sebersole> [11:11] <sebersole> with a caveat
[18:14] <sebersole> [11:12] <sebersole> "hey... EntityKey *can* act as a EntityCacheKey, but only in non-multi-tenant environments"
[18:14] <sebersole> :)
[18:15] <sannegrinovero> ok, I was thinking of "conceptually" not to actually have it implement the Java interface
[18:15] <sebersole> too bad we cant groovy this :)
[18:15] <sannegrinovero> wouldn't it then be confusing? One might think he can pass the EntityKey directly into the caching methods, without invoking the createEntityCacheKey method first to create the "right kind" of EntityCacheKey
[18:16] <sebersole> well that would be a bug right?
[18:16] <sebersole> ;)
[18:16] <sannegrinovero> yes we could catch it with tests
[18:16] <sebersole> look
[18:16] <sebersole> like i said, that part is not my primary design goal here
[18:17] <sebersole> but
[18:19] <sebersole> it is possible if we just have EntityKey implement EntityCacheKey
[18:19] <sebersole> its not ideal
[18:21] <sebersole> we could also use reflection based java duck typing
[18:21] <sebersole> to make the EntityKey act like a EntityCacheKey in those cases
[18:22] <sebersole> not sure how well that works with serialization etc though
[18:23] <sannegrinovero> I think we could leave this lust detail open
[18:23] <sebersole> seems like an awful lot os hacks and drawbacks, which to me generally shows that you are trying to fit a square peg oin round hole
[18:23] <sannegrinovero> the contract changes seem clear
[18:23] <sannegrinovero> right, which is why above I was inclined to say we wouldn't benefit from this last point for instance reuse, but really this last one is the lowest value
[18:24] <sebersole> as long as you are not sayin that method should not exist, ok
[18:24] <sannegrinovero> so I'd vote to keep the EntityKey contract "clean", let the implementor figure out a clever workaround if he wishes so
[18:25] <sannegrinovero> no the method is fine
[18:25] <sannegrinovero> it looks good and fits the purpose
[18:25] <sebersole> i agree that duck typing EntityKey into an EntiytCacheKey is too much
[18:25] <sebersole> right
[18:25] <sannegrinovero> BTW the implementor could try casting the key to its private implementation - it might have been self-generated - 
[18:25] <sannegrinovero> and his custom implementation *could* be able to fullfill the other contract too..
[18:26] <sannegrinovero> just not ORM's problem ;)
[18:26] <sannegrinovero> thanks sebersole, I'll log this on JIRA there is lots of good ideas. I'll see how far I can apply it to reality :)
[18:26] <sebersole> nice!
[18:26] <sebersole> looking forward to it
[18:26] <sebersole> love good design discussions :)
[18:26] <sannegrinovero> +1 !