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

GEODE-4299: refactor eviction #1290

Merged

Conversation

dschneider-pivotal
Copy link
Contributor

The EvictionController classes are no longer Serializable nor Cloneable. These internal classes used to be external and needed to be these things in the past because they were stored in the configuration. But they no longer are so these interfaces were removed for clarity.
The EvictionController classes are no longer responsible for creating InternalEvictionStatistics. Instead the other entities it depends on are passed in to its constructor.

The only class with an EvictionController instance is now VMLRURegionMap which makes the code easier to understand and reduces the number of inter-module dependencies.

renamed internalSetMaximum to setMaximum on EvictionAttributesImpl

AbstractBucketRegionQueue no longer uses EvictionStatistics to notify itself that it removed some objects from the queue. So it no longer needs to access all the EvictionStatistics.

AbstractLRURegionMap has been moved to VMLRURegionMap
Casts to AbstractLRURegionMap have been removed by added additional methods to RegionMap.

new EvictableRegion interface make clear the things a region implementation needs to do/provide for the eviction implementation

EvictionAttributesMutator is now implemented by the dedicated class EvictionAttributesMutatorImpl

close and clear on RegionMap now take a BucketRegion parameter so that the eviction classes do not need to keep track of the BucketRegion.

EvictionList no longer does some of the things it did before that should have been the responsibility of the EvictionController. Now the EvictionList is given the EvictionController it should defer to. The EvictionListBuilder has been simplified since EvictionList now only needs a controller.

The eviction classes that track statistics have been refactored to be clearer. Now the EvictionStats interface is implemented by classes that provide vsd type stats. The EvictionCounters interface is implemented by a class that wraps EvictionStats and provides atomics for some of them.

Serializable, Declarable, and Clonable.
instead of that class plus AbstractRegion and EvictionAttributesImpl
UnsupportOperationException. It now just returns 0
if it is a HeapLRU.
and the bucket is not recursively closed
code currently does not compile
Copy link
Contributor

@kirklund kirklund left a comment

Choose a reason for hiding this comment

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

+1 Looks good!

@@ -14,816 +14,15 @@
*/
package org.apache.geode.internal.cache;
Copy link

Choose a reason for hiding this comment

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

Maybe we should just go ahead and remove the class now: if we remove it later, we will have the same issue of possible merge conflicts caused by the change.

public interface EvictableRegion {
EvictionAttributes getEvictionAttributes();

boolean getOffHeap();
Copy link

Choose a reason for hiding this comment

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

Should this be called "isOffHeap"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was a preexisting method on the implementation classes that I just added to this new interface. So for this PR I don't plan on renaming it. I think it comes from the external RegionAttributes interface so renaming it could break existing user's code.

Copy link

Choose a reason for hiding this comment

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

makes sense

@dschneider-pivotal dschneider-pivotal merged commit c9feba3 into apache:develop Jan 18, 2018
@dschneider-pivotal dschneider-pivotal deleted the newEvictionRefactor branch January 18, 2018 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants