Ad timestamps (1): I haven't asked you to remove these two methods, I've asked to remove txTimestamp from RegionAccessStrategy.get and other RegionAccess methods, because there's already the SharedSessionContractImplementor session argument which provides that value in the SharedSessionContractImplementor.getTimestamp() method - therefore, repeating this in each method args seemed unnecessary. My apologies, I was a bit vague about this in the comment.
Yep, as I said: " So we can certainly remove passing it around in the cache SPI calls as method args. Perhaps that is all you meant?" I think we should plan on reoving those args then. As an extra bonus that would better document the intent of this timestamp. On SharedSessionContractImplementor#getTimestamp we already document the fact that this is meant to be a timestamp that is prior to the start of the last transaction for the Session; we should expand on that some to be very clear about the intentions of that.
I've mentioned that I find relying on a wall-clock time for consistency pretty unsafe practice, though Infinispan implementation does that, too. ORM's use of these timestamps is perfectly fine AFAIK.
As you said, I think the use of "JVM clock time" is perfectly valid for this use case as discussed above.
Ad timeout (2): Sorry, I could not find what you are referring to :-/
I am referring to Region#getTimeout. ATM all look ups for "timeout" go through a Region (that's where the method is defined). I am just asking whether there is any need for this to be on Region (which implies it can be different for different Regions), or whether we should move that to RegionFactory (aka, it is more general to the provider)?
Ad stats (3): I agree that these may not be meaningful (and I am happy to see toMap going out - that's not even statistics). But users will ask for something like that, so the path should be paved - providing that as optional part is fine.
The only one that I feel strongly about is #toMap. I know people implementing 2nd level cache providers before have complained about this one. IMO it should go away - I think it serves no purpose to expose this through Hibernate's caching SPI. On a related note, I would like to suggest y'all consider "unwrap" capabilities to allow users to get to your specific APIs. I mean something like adding RegionFactory#unwrap, Region#unwrap and/or RegionAccess#unwrap
I am just unsure how should the StatisticsImplementor be used, and how will user figure out how many entities are stored in 2LC or its part.
You mean the StatisticsImplementor I am injecting into StatisticsAwareRegionAccess? The intent of that is to have the Region/RegionAccess take over responsibility for managing reporting lookups, hits and misses to the statistics implementor (IMO this is where that belongs). For the moment I just pass in StatisticsImplementor, but ultimately I think a "RegionObserver" contract makes more sense. Although, if we continue to support #getSizeInMemory, #getElementCountInMemory and #getElementCountOnDisk then that changes the dynamic a bit. Injecting the StatisticsImplementor/RegionObserver operates on a push model in regards to letting the Region/RegionAccess manage notifications to statistics about lookups/hits/misses. But #getSizeInMemory, #getElementCountInMemory and #getElementCountOnDisk are inherently more pull related. Perhaps we just handle these (lookup/hit/miss counts versus size-in-memory/element-count...) separately - especially if we say the latter ones are "optional". Relatedly, at the moment I inject the StatisticsImplementor into the RegionAccess. Logically however the statistics are per Region. So I'd actually propose that we move that injection target to Region. The RegionAccess is still the right place to recognize hits/misses/etc, but it can simply delegate to its Region's RegionObserver. Keep in mind too that statistics report this information per-Region. That means from the perspective of statistics lookup count, hit count, miss count, size in memory, etc are all relative to Region. Having the Region be able to answer especially these optional #getSizeInMemory, #getElementCountInMemory and #getElementCountOnDisk means the Region would have to keep track of all the RegionAccess objects built from it (so that it can group all those values together). One option to remove this from cache-specific statistics (org.hibernate.stat.SecondLevelCacheStatistics) and instead expose it through the entity/collection/natural-id statistics (EntityStatistics, CollectionStatistics, NaturalIdStatistics)
Ad SessionFactoryOptions: yes, to resolve settings.getServiceRegistry().getService( StrategySelector.class ).resolveDefaultableStrategy(...) and then also to access user-provided configuration file using proper class loader.
Ok, but those are easily handleable in other ways... RegionFactory is itself a Service. Services can be injected with the ServiceRegistry they belong to. |