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

Remove map eviction policy #15939

Merged

Conversation

@ahmetmircik
Copy link
Member

ahmetmircik commented Nov 5, 2019

Instead we will use EvictionConfig class to introduce custom eviction policies.

follow-up PR of #15592

also closes #15614

Changes in this PR:

  • Removed MapEvictionPolicy --> added MapEvictionPolicyComparator
  • Converted EvictionPolicyComparator, MapEvictionPolicyComparator and
    CacheEvictionPolicyComparator to an interface type.
  • Moved internal api EvictionPolicyComparator&EvictableEntryView to spi.eviction package
  • Relevant client protocol changes
  • Renamed accessHits --> hits

EE: hazelcast/hazelcast-enterprise#3328

@ahmetmircik ahmetmircik added this to the 4.0 milestone Nov 5, 2019
@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/removeMapEvictionPolicy branch 2 times, most recently from 87972e9 to 0bdf796 Nov 5, 2019
@ahmetmircik ahmetmircik requested review from mmedenjak and vbekiaris Nov 6, 2019
@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/removeMapEvictionPolicy branch from 0bdf796 to b32035a Nov 6, 2019
@ahmetmircik ahmetmircik marked this pull request as ready for review Nov 6, 2019
@ahmetmircik ahmetmircik requested a review from hazelcast/clients as a code owner Nov 6, 2019
@ahmetmircik

This comment has been minimized.

Copy link
Member Author

ahmetmircik commented Nov 6, 2019

run-lab-run

@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/removeMapEvictionPolicy branch 9 times, most recently from ec4278a to a26c13c Nov 6, 2019
@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/removeMapEvictionPolicy branch from a26c13c to 10d134f Nov 12, 2019
@ahmetmircik

This comment has been minimized.

Copy link
Member Author

ahmetmircik commented Nov 12, 2019

re-opening, closed mistakenly.

@ahmetmircik ahmetmircik reopened this Nov 12, 2019
@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/removeMapEvictionPolicy branch 2 times, most recently from 5776442 to fb00008 Nov 12, 2019
@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/removeMapEvictionPolicy branch from caca366 to a401ac2 Nov 12, 2019
Copy link
Contributor

mmedenjak left a comment

Checked the whole PR, it looks great with only the last comments I've left. I didn't know we even had this duplication until now :)

One thing that might be addressed in a follow-up PR - EntryView can be renamed to MapEntryView (since it's mostly used by IMap and replicated map) and moved to com.hazelcast.map

@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/removeMapEvictionPolicy branch from 29a10a1 to 516d55e Nov 13, 2019
@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/removeMapEvictionPolicy branch 2 times, most recently from 8e8878f to e559390 Nov 14, 2019
@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/removeMapEvictionPolicy branch from 2ccf46b to 7283517 Nov 14, 2019
Copy link
Contributor

vbekiaris left a comment

Nice job!

String mapEvictionPolicyClassName = "com.hazelcast.map.eviction.LRUEvictionPolicy";
String xml = HAZELCAST_START_TAG
+ "<map name=\"test\">"
+ "<map-eviction-policy-class-name>" + mapEvictionPolicyClassName + "</map-eviction-policy-class-name> "

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris Nov 15, 2019

Contributor

is there another test covering map eviction configured with custom comparator class name?

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Nov 15, 2019

Author Member

added a specific test for this case: 1576a68

+ "hazelcast:\n"
+ " map:\n"
+ " test:\n"
+ " map-eviction-policy-class-name: " + mapEvictionPolicyClassName;

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris Nov 15, 2019

Contributor

same as above for XmlConfigBuilderTest

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Nov 15, 2019

Author Member

added a specific test for this case: 1576a68

@ahmetmircik ahmetmircik merged commit fbca5a3 into hazelcast:master Nov 15, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@ahmetmircik ahmetmircik deleted the ahmetmircik:fix/4.0/removeMapEvictionPolicy branch Nov 15, 2019
petrpleshachkov pushed a commit to petrpleshachkov/hazelcast that referenced this pull request Nov 19, 2019
Fixed a regression caused by
hazelcast#15939 so that
MapEvictionPolicyComparator has no proper equals/hasCode methods
implementation. As a result, EvictionConfig#equals returns wrong result.

Fixes: hazelcast#16035
petrpleshachkov pushed a commit to petrpleshachkov/hazelcast that referenced this pull request Nov 19, 2019
Fixed a regression caused by
hazelcast#15939 so that
MapEvictionPolicyComparator has no proper equals/hasCode methods
implementation. As a result, EvictionConfig#equals returns wrong result.

Fixes: hazelcast#16035
petrpleshachkov pushed a commit to petrpleshachkov/hazelcast that referenced this pull request Nov 19, 2019
Fixed a regression caused by
hazelcast#15939 so that
MapEvictionPolicyComparator has no proper equals/hasCode methods
implementation. As a result, EvictionConfig#equals returns wrong result.

Fixes: hazelcast#16035
petrpleshachkov added a commit that referenced this pull request Nov 20, 2019
Fixed a regression caused by
#15939 so that
MapEvictionPolicyComparator has no proper equals/hasCode methods
implementation. As a result, EvictionConfig#equals returns wrong result.

Fixes: #16035
petrpleshachkov added a commit to petrpleshachkov/hazelcast that referenced this pull request Nov 22, 2019
Fixed a regression caused by
hazelcast#15939 so that
MapEvictionPolicyComparator has no proper equals/hasCode methods
implementation. As a result, EvictionConfig#equals returns wrong result.

Fixes: hazelcast#16035
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.