Re: Review Request 53652: GEODE-2089 entry-idle-time setting on the client side cache is not working as expected

2016-11-10 Thread Udo Kohlmeyer 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53652/#review155653
---


Ship it!




Ship It!

- Udo Kohlmeyer


On Nov. 10, 2016, 9: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, 9: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
> 
>

Re: Review Request 53652: GEODE-2089 entry-idle-time setting on the client side cache is not working as expected

2016-11-10 Thread Bruce Schuchardt 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53652/
---

(Updated Nov. 10, 2016, 9:47 p.m.)


Review request for geode, Darrel Schneider, Hitesh Khamesra, and Udo Kohlmeyer.


Changes
---

renamed setLastModified(long,long) to 
setLastModifiedAndAccessedTimes(long,long) and added javadocs


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 (updated)
-

  
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

Re: Review Request 53652: GEODE-2089 entry-idle-time setting on the client side cache is not working as expected

2016-11-10 Thread Bruce Schuchardt 


> 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> 
>

Re: Review Request 53652: GEODE-2089 entry-idle-time setting on the client side cache is not working as expected

2016-11-10 Thread Udo Kohlmeyer 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53652/#review155638
---




geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionEntry.java
 (lines 170 - 172)


I think this overloaded method is a little confusing. It is setting the 
lastModified but takes lastAccessed as well. Then we only use lastModified.



geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionEntry.java
 (lines 178 - 180)


maybe we use setLastAccessed here instead of the overloaded setLastModified



geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionEntry.java
 (lines 178 - 180)


I think this implementation should read

```setLastModified(lastModified);
setLastAccessed(lastAccessed);``` 

Then the VMStatsDiskLRURegionEntryHeapIntKey can override 
`updateStatsForPut` with its custom implementation.



geode-core/src/main/java/org/apache/geode/internal/cache/VMStatsDiskLRURegionEntryHeapIntKey.java
 (lines 306 - 312)


maybe the !DISABLE_ACCESS_TIME_UPDATE_ON_PUT logic should reside in the 
updateStatsForPut method, or at least override the normal behaviour from 
AbstractRegionEntry.


- Udo Kohlmeyer


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
> 
>

Re: Review Request 53652: GEODE-2089 entry-idle-time setting on the client side cache is not working as expected

2016-11-10 Thread Darrel Schneider 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53652/#review155633
---


Ship it!




Ship It!

- Darrel Schneider


On Nov. 10, 2016, 8:47 a.m., Bruce Schuchardt wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53652/
> ---
> 
> (Updated Nov. 10, 2016, 8:47 a.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
> 
>