[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-2896?page=c... ] Steve Ebersole updated HHH-2896: -------------------------------- Fix Version/s: 3.6 -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://opensource.atlassian.com/projects/hibernate/secure/Administrators.... - For more information on JIRA, see: http://www.atlassian.com/software/jira

[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-2896?page=c... ] Steve Ebersole updated HHH-2896: -------------------------------- Attachment: hhh-2896_irc_discussion.txt Attached IRC discussion about design of this -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-2896?page=c... ] Steve Ebersole edited comment on HHH-2896 at 9/27/11 5:24 PM: -------------------------------------------------------------- Caching will require care. Is special handling for persistence context needed? {quote} <sebersole> now far and away the hardest part of implementing this will be caching <sebersole> both <sebersole> and <EricDalquist> right, since the whole point of natural id lookups is better caching behavior <sebersole> if we leave NaturalIdLoadAccess#getReference() we also need proxies to understand this <sebersole> unless we "partially resolve" the natural key <sebersole> thats probably the better option\ <sebersole> actually thats the only real option <sebersole> we will need to cross-reference the natural key <sebersole> to build the proxy <sebersole> EricDalquist: still on board? <EricDalquist> and this is where I'm a little more lost. My understanding right now is that the naturalid query looks in the query cache for data and then in the 2nd level cache for the partially-assembled object data <EricDalquist> and if I'm following when you say "cross-reference the natural key" you're meaning we need to have some cache of "natural id" -> pk <EricDalquist> and then we use to pk to load the actual object <sebersole> well basically what it does (or tries to do) is to handle this cross ref <sebersole> thats the "looks in the query cache" bit <sebersole> EricDalquist: exactly what i mean <EricDalquist> so are you thinking a whole new cache or continuing to essentially overload the query cache? <sebersole> i'd opt for a whole new cache <sebersole> using the query cache for this purpose causes problems <EricDalquist> I would too from an implementers pov, then I can tune it separately from the query cache <sebersole> that too <sebersole> like you may or may not want to distribute this cached data <EricDalquist> right <sebersole> whereas thats frowned upon for query results <sebersole> but even more basic, i meant it lets you better handle invalidating the entries <EricDalquist> yup <EricDalquist> especially if we can just store the base primitives involved in the natural id and pk <sebersole> you *have* to in fact <EricDalquist> right, I remember seeing the bug a while back about cache keys having references to assembled objects :/ <EricDalquist> so the workflow would look like: Lookup pk from NaturalIdCache, On hit load object by PK <sebersole> yes, you have to dissassemble the values <EricDalquist> or: Lookup pk from NaturalIdCache, On miss query for pk by naturalid? <EricDalquist> for that on-miss logic would the code just go through the normal query generation code path? <sebersole> Lookup correlated pk from NaturalIdCache. on miss, query db for related key, add to cache. load by pk {quote} was (Author: steve): Caching will require care. Is special handling for persistence context needed? {quote} <sebersole> now far and away the hardest part of implementing this will be caching <sebersole> both <sebersole> and <EricDalquist> right, since the whole point of natural id lookups is better caching behavior <sebersole> if we leave NaturalIdLoadAccess#getReference() we also need proxies to understand this <sebersole> unless we "partially resolve" the natural key <sebersole> thats probably the better option\ <sebersole> actually thats the only real option <sebersole> we will need to cross-reference the natural key <sebersole> to build the proxy <sebersole> EricDalquist: still on board? <EricDalquist> and this is where I'm a little more lost. My understanding right now is that the naturalid query looks in the query cache for data and then in the 2nd level cache for the partially-assembled object data <EricDalquist> and if I'm following when you say "cross-reference the natural key" you're meaning we need to have some cache of "natural id" -> pk <EricDalquist> and then we use to pk to load the actual object <sebersole> well basically what it does (or tries to do) is to handle this cross ref <sebersole> thats the "looks in the query cache" bit <sebersole> EricDalquist: exactly what i mean <EricDalquist> so are you thinking a whole new cache or continuing to essentially overload the query cache? <sebersole> i'd opt for a whole new cache <sebersole> using the query cache for this purpose causes problems <EricDalquist> I would too from an implementers pov, then I can tune it separately from the query cache <sebersole> that too <sebersole> like you may or may not want to distribute this cached data <EricDalquist> right <sebersole> whereas thats frowned upon for query results <sebersole> but even more basic, i meant it lets you better handle invalidating the entries <EricDalquist> yup * gustavonalle has quit (Quit: gustavonalle) <EricDalquist> especially if we can just store the base primitives involved in the natural id and pk <sebersole> you *have* to in fact <EricDalquist> right, I remember seeing the bug a while back about cache keys having references to assembled objects :/ <EricDalquist> so the workflow would look like: Lookup pk from NaturalIdCache, On hit load object by PK <sebersole> yes, you have to dissassemble the values <EricDalquist> or: Lookup pk from NaturalIdCache, On miss query for pk by naturalid? <EricDalquist> for that on-miss logic would the code just go through the normal query generation code path? <sebersole> Lookup correlated pk from NaturalIdCache. on miss, query db for related key, add to cache. load by pk {quote} -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-2896?page=c... ] Steve Ebersole commented on HHH-2896: ------------------------------------- We also should decide about immutable keys here. Current code tries to handle the natural key value changing in the db, but that causes huge problems. And I contend that just like other caching, this is not Hibernate's responsibility to manage this. Also, we need to outline invalidity on changes done through Hibernate. -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-2896?page=c... ] Eric Dalquist edited comment on HHH-2896 at 9/28/11 2:34 PM: ------------------------------------------------------------- SessionImpl will be restructured to make IdentifierLoadAccess the first-class method of retrieving entities by primary key. All existing load and get methods will be updated to delegate to IdentifierLoadAccess. The initial implementation of NaturalIdLoadAccess will be a two-step process which includes the creation of a new ResolveNaturalIdEvent event type. # Fire a ResolveNaturalIdEvent with the entity name and natural id attribute values. This event will resolve the primary key for the entity. # Fire a LoadEvent using the resolved primary key and provided lock options. A new cache to store the NaturalId->PK mappings is needed, it will be named [entityName].[naturalId]. This cache will be similar to a query cache in functionality, lookups by immutable natural id will never do an isUpToDate check, lookups on mutable natural id's will behave very similarly to existing query cached lookups. The hibernate persist logic will need to be reviewed for its handling of immutable natural id attributes. These attributes should never be included in generated SQL update statements to enforce the immutability constraint of the natural id. New methods to manage this new cache will need to be added to org.hibernate.Cache {code:java} evictNaturalKeyEntry(String entityName, Object key) evictNaturalKeyRegion(String entityName) {code} Improvements that can be planned out beyond the initial feature implementation * Extend the handling of ResolveNaturalIdEvent to load the entity into the session cache if it is not already present, this should reduce the number of db queries by 1 for each naturalid load that is a cache-miss. * Add handling of the various persist/load/merge/delete events to maintain the NaturalID->PK mappings in this new cache. was (Author: edalquist): SessionImpl will be restructured to make IdentifierLoadAccess the first-class method of retrieving entities by primary key. All existing load and get methods will be updated to delegate to IdentifierLoadAccess. The initial implementation of NaturalIdLoadAccess will be a two-step process which includes the creation of a new ResolveNaturalIdEvent event type. # Fire a ResolveNaturalIdEvent with the entity name and natural id attribute values. This event will resolve the primary key for the entity. # Fire a LoadEvent using the resolved primary key and provided lock options. A new cache to store the EntityName.[NaturalId]->PK mappings is needed. This cache will be similar to a query cache in functionality, lookups by immutable natural id will never do an isUpToDate check, lookups on mutable natural id's will behave very similarly to existing query cached lookups. The hibernate persist logic will need to be reviewed for its handling of immutable natural id attributes. These attributes should never be included in generated SQL update statements to enforce the immutability constraint of the natural id. New methods to manage this new cache will need to be added to org.hibernate.Cache {code:java} evictNaturalKeyEntry(String entityName, Object key) evictNaturalKeyRegion(String entityName) {code} Improvements that can be planned out beyond the initial feature implementation * Extend the handling of ResolveNaturalIdEvent to load the entity into the session cache if it is not already present, this should reduce the number of db queries by 1 for each naturalid load that is a cache-miss. * Add handling of the various persist/load/merge/delete events to maintain the NaturalID->PK mappings in this new cache. -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-2896?page=c... ] Steve Ebersole commented on HHH-2896: ------------------------------------- I wonder if the following method also makes sense on {{org.hibernate.Cache}}: {code} evictNaturalKeyRegions() {code} -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-2896?page=c... ] Eric Dalquist commented on HHH-2896: ------------------------------------ Here is a diff of progress so far: https://github.com/edalquist/hibernate-core/compare/master...HHH-2879 The two new *LoadAccess APIs are in place Existing load and get methods on Session are delegating to the new APIs A new ResolveNaturalIdEvent exists as well as the corresponding listener and default impl The current impl only does direct from DB loading of the entity id based on the natural-id, no query or session caching is done yet Basic testing showing it working I'd love to get a review at this point with some feedback to see if I'm on the right track here. -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-2896?page=c... ] Steve Ebersole updated HHH-2896: -------------------------------- Fix Version/s: (was: 4.1.0) 5.0.0 -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-2896?page=c... ] Steve Ebersole updated HHH-2896: -------------------------------- Fix Version/s: (was: 3.x) -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-2896?page=c... ] Steve Ebersole closed HHH-2896. ------------------------------- Resolution: Fixed Fix Version/s: (was: 4.1.0) -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira