GEODE-3764: prevent early idle expiration #940
Conversation
dschneider-pivotal
requested review from
agingade,
kirklund,
lgallinat,
nreich and
pivotal-eshu
| long latestLastAccessTime = getLatestLastAccessTimeOnOtherMembers(); | ||
| if (latestLastAccessTime > getLastAccessedTime()) { | ||
| setLastAccessedTime(latestLastAccessTime); | ||
| return false; |
There was a problem hiding this comment.
Is this always true? Can last accessed time from another member can be newer than ours, but still be expired? Do we need to test this new value against the expiration time point?
There was a problem hiding this comment.
See the new commit that addresses this issue.
If a later lastAccessTime is obtained from others, the caller of this method now rechecks if it is still expired or if it should be rescheduled. This will prevent an extra round of messages in the case in which two members have slightly different lastAccessTimes but are both expired.
nreich
left a comment
nreich
left a comment
There was a problem hiding this comment.
Looks good after latest change (other than spotless validation failure that I assume you will fix before merge).
| } | ||
| } | ||
|
|
||
| public long getLatestLastAccessTime() { |
| VM[] vms = new VM[] {member1, member2, member3}; | ||
| for (VM vm : vms) { | ||
| vm.invoke(() -> { | ||
| KEEP_READING.set(false); |
There was a problem hiding this comment.
This should also be in setUp(), right? Now the first test method sees the KEEP_READING value to be true and other subsequent tests (if added in future) sees it set to FALSE...
There was a problem hiding this comment.
KEEP_READING is now initialized to true in setUp
| long expTime = getExpirationTime(); | ||
| if (expTime > 0L && getNow() >= expTime) { | ||
| return true; | ||
| if (expTime > 0L) { |
There was a problem hiding this comment.
What should happen if expiration is due to ttl?
There was a problem hiding this comment.
Then it is okay to just expire based on the local information.
Since last modified time is always changed by a write and since writes go to every copy of the data the ttl check only needs to happen locally.
The way this works in this fix is that the implementation of isIdleExpiredOnOthers
does this check:
if (getIdleAttributes().getTimeout() <= 0L) {
// idle expiration is not being used
return true;
}
So if you expiration configuration does not care about idle time then we just return true which means expiration is possible.
If we are using idle time then we fetch it from others and redo this calculation which may cause is to still expire due to ttl (note that you can have both idle and ttl configured on a region).
There was a problem hiding this comment.
@dschneider-pivotal Thanks Darrel. I thought the same that we should be expiring if its TTL.
getExpirationTime() call in the previous line also checks if TTL or Idle and returns expiryTime based on the what is configured.
Probably its more readable if the check is part of this function rather than in isIdleExpiredOnOthers.
Idle expiration now sends a message to others checking if they have been read more recently. If so and if given the new last access time the entry is not expired then the expiration is rescheduled. This is only done for distributed expiration actions. This change applies to both replicates and partitioned regions. The system property "geode.restoreIdleExpirationBehavior" can be set to true to restore the previous idle expiration behavior.
dschneider-pivotal
force-pushed
the
feature/GEODE-3764
branch
from
51ed294 to
84ad803
Compare
5 participants
Idle expiration now sends a message to others checking
if they have been read more recently. If so the expiration
is rescheduled.
This is only done for distributed expiration actions.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.