Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix LRU comparison algorithm #18624

Merged
merged 3 commits into from Jun 15, 2021
Merged

Fix LRU comparison algorithm #18624

merged 3 commits into from Jun 15, 2021

Conversation

ahmetmircik
Copy link
Member

@ahmetmircik ahmetmircik commented May 3, 2021 •
edited by mmedenjak

closes #18614

For lru sorting, when last-access-time is zero, we will use creation-time of the entry. This is to prevent premature removal of lately added but not yet accessed entries.

Backport: #18632
Forward-port: #18909

@ahmetmircik ahmetmircik added this to the 4.2.1 milestone May 3, 2021
@ahmetmircik ahmetmircik changed the title WIP Fix LRU comparison algorithm May 3, 2021
@hazelcast hazelcast deleted a comment from hz-devops-test May 3, 2021
@hz-devops-test
Copy link

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file 
--------------------------
-------TEST FAILURE-------
--------------------------
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   ExpirationTimeTest.last_access_time_updated_on_primary_when_read_backup_data_enabled:451 lastAccessTimeBefore:1620075212000, lastAccessTimeAfter:1620075212000
[ERROR]   ExpirationTimeTest.testExpirationTime_whenMaxIdleTime_isSmallerThan_TTL:352 expected:<3240150432000> but was:<1620075221000>
[ERROR]   ExpirationTimeTest.testLastAccessTime_isZero_afterFirstPut:377 expected:<0> but was:<1620075212000>
[INFO] 
[ERROR] Tests run: 36328, Failures: 3, Errors: 0, Skipped: 988
[INFO] 

[ERROR] There are test failures.

@ahmetmircik ahmetmircik marked this pull request as ready for review May 4, 2021 12:38
@ahmetmircik ahmetmircik mentioned this pull request May 4, 2021
Merged
@mmedenjak mmedenjak added Source: Internal PR or issue was opened by an employee Team: Core labels May 17, 2021
ufukyilmaz
ufukyilmaz reviewed Jun 11, 2021
View reviewed changes
hazelcast/src/test/java/com/hazelcast/map/ExpirationTimeTest.java Outdated
@@ -344,8 +344,7 @@ public void testExpirationTime_whenMaxIdleTime_isSmallerThan_TTL() {
map.put(1, 1, 100, TimeUnit.SECONDS);

EntryView<Integer, Integer> entryView = map.getEntryView(1);
long lastAccessTime = entryView.getLastAccessTime();
long delayToExpiration = lastAccessTime + TimeUnit.SECONDS.toMillis(10);
long delayToExpiration = TimeUnit.SECONDS.toMillis(10);

// lastAccessTime is zero after put, we can find expiration by this calculation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is stale. Could you update it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

ufukyilmaz
ufukyilmaz reviewed Jun 11, 2021
View reviewed changes
...c/main/java/com/hazelcast/internal/eviction/impl/comparator/LRUEvictionPolicyComparator.java
Comment on lines +36 to +37
long time1 = Math.max(e1.getCreationTime(), e1.getLastAccessTime());
long time2 = Math.max(e2.getCreationTime(), e2.getLastAccessTime());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need these comparisons after the change in AbstractRecordStore? I don't know exactly where the last access times can be changed, so it's just a question to learn the cases that need them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comparator is a common one used by multiple data structures, it is not specific to IMap, i have also fixed it here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better if we also apply the change made in AbstractRecordStore to AbstractCacheRecordStore (and all others) to have a convenience on these last access times.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no issue in the context of icache. Setting last-access-time is a missing requirement for imap after 4.2 improvements. No need for others now.

ufukyilmaz
ufukyilmaz approved these changes Jun 11, 2021
View reviewed changes
Copy link
Contributor

@ufukyilmaz ufukyilmaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though setting the initial last access times in cache and map entries in different ways seems rough to me, I leave this discussion to other reviewers. Other than that it looks good to me.

vbekiaris
vbekiaris approved these changes Jun 15, 2021
View reviewed changes
hazelcast/src/test/java/com/hazelcast/internal/eviction/EvictionPolicyEvaluatorTest.java
for (int i = 0; i < recordCount; i++) {
long creationTime = baseTime + (i * 100);
minCreationTime = minCreationTime == -1 ? creationTime : Math.min(creationTime, minCreationTime);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could also initialize minCreationTime = Long.MAX_VALUE and just use minCreationTime = Math.min(creationTime, minCreationTime); here instead of the ternary operator.

@ahmetmircik ahmetmircik merged commit 8a63661 into hazelcast:4.2.z Jun 15, 2021
@ahmetmircik ahmetmircik deleted the 4.2/fix/lruImpl branch June 15, 2021 10:05
ahmetmircik added a commit to ahmetmircik/hazelcast that referenced this pull request Jun 15, 2021
@ahmetmircik
Fix LRU comparison algorithm (hazelcast#18624)
d29fedc
@ahmetmircik ahmetmircik mentioned this pull request Jun 15, 2021
Merged
@mmedenjak mmedenjak removed their request for review June 16, 2021 06:15
This was referenced Jun 16, 2021
Merged
Closed
@ahmetmircik ahmetmircik mentioned this pull request Jun 25, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vbekiaris vbekiaris vbekiaris approved these changes

@ufukyilmaz ufukyilmaz ufukyilmaz approved these changes

Assignees
No one assigned
Labels
Module: IMap Source: Internal PR or issue was opened by an employee Team: Core Type: Defect
Projects
None yet
Milestone
4.2.1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ahmetmircik @hz-devops-test @vbekiaris @ufukyilmaz @mmedenjak