> On Nov. 10, 2016, 8:21 p.m., Udo Kohlmeyer wrote: > > geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionEntry.java, > > lines 170-172 > > <https://reviews.apache.org/r/53652/diff/1/?file=1560532#file1560532line170> > > > > I think this overloaded method is a little confusing. It is setting the > > lastModified but takes lastAccessed as well. Then we only use lastModified.
I'll change the name of the method to be more understandable. > On Nov. 10, 2016, 8:21 p.m., Udo Kohlmeyer wrote: > > geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionEntry.java, > > lines 178-180 > > <https://reviews.apache.org/r/53652/diff/1/?file=1560532#file1560532line178> > > > > I think this implementation should read > > > > ```setLastModified(lastModified); > > setLastAccessed(lastAccessed);``` > > > > Then the VMStatsDiskLRURegionEntryHeapIntKey can override > > `updateStatsForPut` with its custom implementation. AbstractRegionEntry doesn't have a setLastAccessedTime nor a lastAccessed instance variable. Other subclasses of AbstractRegionEntry also do not have lastAccessedTime. I'll change the name of this method so it's clearer and post an update. > On Nov. 10, 2016, 8:21 p.m., Udo Kohlmeyer wrote: > > geode-core/src/main/java/org/apache/geode/internal/cache/VMStatsDiskLRURegionEntryHeapIntKey.java, > > lines 306-312 > > <https://reviews.apache.org/r/53652/diff/1/?file=1560539#file1560539line306> > > > > maybe the !DISABLE_ACCESS_TIME_UPDATE_ON_PUT logic should reside in the > > updateStatsForPut method, or at least override the normal behaviour from > > AbstractRegionEntry. setLastModified is also invoked directly from transaction code in AbstractRegionMap. Moving this to updateStatsForPut would change behavior for transactions, so I don't think we should mess with that. - Bruce ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53652/#review155638 ----------------------------------------------------------- On Nov. 10, 2016, 4:47 p.m., Bruce Schuchardt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53652/ > ----------------------------------------------------------- > > (Updated Nov. 10, 2016, 4:47 p.m.) > > > Review request for geode, Darrel Schneider, Hitesh Khamesra, and Udo > Kohlmeyer. > > > Bugs: GEODE-2089 > https://issues.apache.org/jira/browse/GEODE-2089 > > > Repository: geode > > > Description > ------- > > When we pull an entry in from another cache in > LocalRegion.findObjectInSystem() we end up invoking updateStatsForPut and > then updateStatsForGet. The former method installs a lastModifiedTime from > the version tag that came from the other cache and this is used in > updateStatsForPut to schedule an expiry-task for the entry. Then > updateStatsForGet is invoked to set the lastAccessed time of the entry to the > current time. However, by the time this is done the entry may have already > been removed by the expiry-task if the lastModified time was too far in the > past. > > The fix is to establish both the lastModified and lastAccessed times in > updateStatsForPut. > > I've included two of the "leaf" RegionEntry classes in the diff but all of > them had to be modified in the same way. > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionEntry.java > 41ca8d084405480c8e4e69bb99db74e19529bc71 > > geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java > de05b0d3991254834da94ed97ada9c9247aa69ab > geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java > e307c86a0aed48c0304a6a1ef90bdf147a05c379 > > geode-core/src/main/java/org/apache/geode/internal/cache/NonLocalRegionEntry.java > 6de85a1ab20b653e1731e0476ffb559bd093a121 > geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java > 0233b0f32d697c15a1148c95d6549f20997f3cc8 > > geode-core/src/main/java/org/apache/geode/internal/cache/ProxyRegionMap.java > b5a3719ed00a56096582f1adf0b21b61b86563b9 > geode-core/src/main/java/org/apache/geode/internal/cache/RegionEntry.java > 3448d1fba101a14d421584beb91fe216246794ed > > geode-core/src/main/java/org/apache/geode/internal/cache/VMStatsDiskLRURegionEntryHeapIntKey.java > 41bd3346c7a275b5fe83fe1df35ac87cc611dd83 > > geode-core/src/test/java/org/apache/geode/cache30/ClientServerCCEDUnitTest.java > 7cf632f850098c178d4cf23bf116292d89220738 > > Diff: https://reviews.apache.org/r/53652/diff/ > > > Testing > ------- > > New unit test, precheckin > > > Thanks, > > Bruce Schuchardt > >
