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.

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

IMap entries eviction support on the indexes level (#11748) #15136

Conversation

@petrpleshachkov
Copy link
Contributor

petrpleshachkov commented Jun 6, 2019

For on-heap indexes update record's lastAccessTime when it is being accessed
through the index, so that expiration maxIdle logic takes this into account.

An index still may return stale entries, because expiration logic is not thread
safe and runs on the partition thread. However, the impact is limited because
a periodic cleaning task evicts entries anyway.

Part of: #11748
EE: hazelcast/hazelcast-enterprise#3022

Petr Pleshachkov added 2 commits Jun 6, 2019
For on-heap indexes update record's lastAccessTime when it is being accessed
through the index, so that expiration maxIdle logic takes this into account.

An index still may return stale entries, because expiration logic is not thread
safe and runs on the partition thread. However, the impact is limited because
a periodic cleaning task evicts entries anyway.

Part of (#11748)
Fixed a compilation issue which silently was caused by merge with recent
changes.
@petrpleshachkov

This comment has been minimized.

Copy link
Contributor Author

petrpleshachkov commented Jun 6, 2019

run-lab-run


@Override
public Map<Data, QueryableEntry> invoke(Map<Data, QueryableEntry> map) {
if (isExpirable()) {

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Jun 13, 2019

Member

If i understand this part correctly, end result set, it is the set we return to user, should be the right place to update access times, otherwise it is possible to update access times wrongly. For example AndPredicate can contain many predicates but it only cares about smallest result set, if smallest result set has 1 entry and others have 1 million, in this case, we do nearly 1 million access-time updates wrongly where only 1 entry's access-time-update is needed? Is this correct?

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Jun 13, 2019

Author Contributor

This is a good question because it depends on how we define "touch" an entry. If we touch an entry only if it goes to the final result set, we have to make this logic somewhere else. I discovered an issue that when we run a predicate query without indexes we don't update the access time at all (see #15137) and so for predicate queries we still have to define "touch" an entry. Now, the touch an entry even if it is part of the intermediate result. The same applies to the HD indexes case.

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Jun 14, 2019

Member

My expectation is updating only final collection of entries, intermediate result touches should not be counted as access. But better to collect ideas on this.

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Jun 19, 2019

Author Contributor

I have implemented lazy Record's last time access (LTA) update approach so that we update LTA when we get it through an iterator. We still can touch a bit more intermediate results but in general case we are not able to touch only final results (it will be very expensive) because we may know whether an entry goes to the final result only on different member. My solution is a best effort approach which I think is reasonable in this situation.

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Jun 25, 2019

Member

seems better thanks.

Petr Pleshachkov added 2 commits Jun 19, 2019
Take into account the reviewer's comments. Implemented lazy Records's
last access time updater when QueryableEntry is retrieved through an index.
…l-maxidle-aware-indexstore
@petrpleshachkov

This comment has been minimized.

Copy link
Contributor Author

petrpleshachkov commented Jun 19, 2019

Hi guys, I have reviewed your comments and made additional changes. Can you please take a look?

Copy link
Contributor

taburet left a comment

Looks good, well done.

@ahmetmircik ahmetmircik self-requested a review Jun 25, 2019
* {@link QueryableEntry} retrieved through the {@link HashMap#entrySet()} or {@link HashMap#values()}.
*
*/
private static class EvictionAwareHashMapDelegate implements Map<Data, QueryableEntry> {

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Jun 25, 2019

Member

I think using Expiration instead of Eviction allover this PR fits better. Reasoning was same with hazelcast/hazelcast-enterprise#3022 (comment)

Minor changes to take into account final reviewer's comments.
@petrpleshachkov petrpleshachkov merged commit f295c99 into hazelcast:master Jun 26, 2019
1 check failed
1 check failed
default Test `FAIL`ed. http://jenkins.hazelcast.com/job/Hazelcast-pr-builder/1853/
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.