make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI #7456

dschneider-pivotal · 2022-03-17T22:25:42Z

This PR introduces a new external API interface: HeapUsageProvider.
Users can create a class that implements the HeapUsageProvider and configure it as a standard Java service (see java.util.ServiceLoader).
The ResourceManager will use the HeapUsageProvider to enforce it's eviction-threshold and critical-threshold.
The HeapUsageProvider provides geode with two pieces of information:

what is the maximum amount of heap memory
what is the current amount of heap memory being used

The HeapUsageProvider must return a reasonable value when getMaxMemory and getBytesUsed are called. It can also optionally implement notifications that it sends to a listener when it detects a change in used memory.

Geode will look for a service that implements HeapUsageProvider during Cache creation. If it finds one and it throws an exception during construction then this will cause the geode Cache to fail its initialization with the thrown exception. If it does not find one then it will use its own default implementation: MemoryPoolMXBeanHeapUsageProvider. This provider works the same as previous releases of geode.

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 build run 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?

demery-pivotal · 2022-03-21T21:29:27Z

geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java

-    if (builder.length() > 1) {
-      builder.setLength(builder.length() - 2);
+  private static HeapUsageMonitor findHeapUsageMonitor() {
+    for (HeapUsageMonitor heapUsageMonitor : heapUsageMonitorLoader) {


If the implementation's constructor throws an exception, the service loader's hasNext() and next() will throw. If that happens, is it okay for the HeapUsageMonitor constructor to throw, or do you want to catch it and fall back to the default heap usage monitor?

Good question. I think we want to fail with an exception since this indicates they tried to specify a HeapUsageMonitor and its constructor failed. An exception thrown here will cause the cache initialization to fail which seems good.

stats are no longer used to detect memory changes. Instead it consistently uses notification from JMX and a polling thread. The polling thread used to be used only if stats were disabled. Also the polling thread now runs in the InternalResourceManager's scheduledExecutor instead of a new executor being created. The default number of threads in that executor has changed from 1 to 2. removed unneeded static fields from the HeapUsageProvider impl. Added a ServerLoader search for the HeapUsageProvider The TenuredHeapConsumptionMonitor is now part of the MemoryPoolMXBeanHeapUsageProvider internals instead of it being create in the InternalResourceManager.

…L system property since "usageThreshold" is no longer used it is no longer required "collectionUsageThreshold" support is now optional

upthewaterspout · 2022-03-22T21:56:00Z

On general comment before I dig in a bit deeper - it looks like this will only work correctly if there is only 1 implementation of this interface on the classpath?

It might be better to have a way for a user to choose which implementation of HeapUsageMonitor they want - or perhaps even for the HeapUsageMonitor to itself decide if it is relevant for the current JVM settings?

…testable

DonalEvans

Given this adds a new public API, there should probably be a docs review of the wording and content of the javadocs on it. Also, it would be good to have some test coverage showing that a user can supply a non-default HeapUsageProvider and have it be picked up and used by Geode.

DonalEvans · 2022-03-22T23:18:33Z

...tegrationTest/java/org/apache/geode/internal/cache/control/MemoryMonitorIntegrationTest.java

    try {
      emitter.removeNotificationListener(
-          InternalResourceManager.getInternalResourceManager(cache).getHeapMonitor());
+          InternalResourceManager.getInternalResourceManager(cache).getHeapMonitor()
+              .getHeapUsageMonitor());
      assertTrue("Expected that the resource manager was not registered", false);
-    } catch (ListenerNotFoundException ignored) {
+    } catch (ListenerNotFoundException expected) {
    }


While you're modifying this class, it would be much better to have this (and other similar code in this class) be an assertThatThrownBy() rather than a try/catch with an "assert that true is false" to force the test to fail if we don't throw.

DonalEvans · 2022-03-22T23:20:13Z

geode-core/src/main/java/org/apache/geode/internal/cache/control/InternalResourceManager.java

-  final int MAX_RESOURCE_MANAGER_EXE_THREADS =
-      Integer.getInteger(GeodeGlossary.GEMFIRE_PREFIX + "resource.manager.threads", 1);
+  private final int MAX_RESOURCE_MANAGER_EXE_THREADS =
+      Integer.getInteger(GeodeGlossary.GEMFIRE_PREFIX + "resource.manager.threads", 2);


Why is this default value being increased?

The poller thread used to create its own ScheduledPoolExecutor and now it uses this existing one on the InternalResourceManager. I changed the default number of threads from 1 to 2 to decrease the changes that the poller would be stuck behind other operations that use this executor. We could go back to having a dedicated executor.

geode-core/src/main/java/org/apache/geode/cache/control/HeapUsageProvider.java

...re/src/distributedTest/java/org/apache/geode/cache/management/MemoryThresholdsDUnitTest.java

...grationTest/java/org/apache/geode/internal/statistics/GemFireStatSamplerIntegrationTest.java

DonalEvans · 2022-03-22T23:46:13Z

...src/main/java/org/apache/geode/internal/cache/control/MemoryPoolMXBeanHeapUsageProvider.java

+  private final Supplier<List<MemoryPoolMXBean>> memoryPoolSupplier;
+  private final Supplier<NotificationEmitter> notificationEmitterSupplier;
+  private final TenuredHeapConsumptionMonitor tenuredHeapConsumptionMonitor;
+  // JVM MXBean used to report changes in heap memory usage
+  private final MemoryPoolMXBean tenuredMemoryPoolMXBean;
+  // Calculated value for the amount of JVM tenured heap memory available.
+  private final long tenuredPoolMaxMemory;
+
+  private volatile LongConsumer heapUsageListener = disabledHeapUsageListener;


Just personal preference, but could these fields be moved to be above the private static methods please? That seems to be the approach we're following elsewhere in the codebase.

dschneider-pivotal · 2022-03-23T00:08:30Z

@upthewaterspout Geode's ResourceManager is a singleton and it will only have one instance of HeapUsageProvider. Geode has an implementation of HeapUsageProvider on the class path but it is not a service. Geode's instance handles all the JVMs and GCs we support. The only reason for defining a service that implements HeapUsageProvider is if you want to replace geode's built in one. So at that point you will have two on the class path and only the service one will be used.
If you define multiple services that implement HeapUsageProvider then only the first one returned by the ServiceLoader will be constructed and used. We could just document this behavior. I don't see a need to make it more complicated. I'm not that familiar with the ServiceLoader so it would be good to know if this is a strange way of using java services.

kohlmu-pivotal · 2022-03-23T02:05:25Z

geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java

+  @Immutable
+  private static final ServiceLoader<HeapUsageProvider> heapUsageMonitorLoader =
+      ServiceLoader.load(HeapUsageProvider.class);


This is a novel approach to service loading. Why would we have the ServiceLoading on static initialization of the class but the looking up of the implementation only at construction time of the HeapMemoryMonitor? Given that you limit it to the first implementation found AND on construction time of the HeapMemoryMonitor, I believe all this code could be done in the same location. i.e in the findHeapUsageMonitor method

I'm still not sure we should even use the ServiceLoader for this since it supports multiple instances and we only want one. Would a system property that they set to the class name of their implementation be better?

I think a ServiceLoader works perfectly well. Having an SPI can lead to potentially having multiple on the classpath. A thought could be that one could introduce the idea of "loadWithIdentifier"... Similarly to how SSL works. There are multiple implementations on the classpath and one needs to specify which one to load. I think this could support us rather well, if we want to support different GC algorithms. Thoughts?

kohlmu-pivotal · 2022-03-23T02:15:52Z

geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java

-  public static MemoryPoolMXBean getTenuredMemoryPoolMXBean() {
-    if (tenuredMemoryPoolMXBean != null) {
-      return tenuredMemoryPoolMXBean;
+  private static HeapUsageProvider findHeapUsageMonitor() {


Could we possibly change this to a "getHeapUsageMonitor"... It is not really a "find" method

I don't really like "get" either since to me that makes it seem like it is returning a value that already exists. How about "createHeapUsageProvider"?

Maybe we take a leaf out of the ServiceLoaders and call it "load"...

kohlmu-pivotal · 2022-03-23T02:16:28Z

geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java


  HeapMemoryMonitor(final InternalResourceManager resourceManager, final InternalCache cache,
-      final ResourceManagerStats stats,
-      TenuredHeapConsumptionMonitor tenuredHeapConsumptionMonitor) {
+      final ResourceManagerStats stats) {


Why would we not pass in the correct HeapUsageProvider as a parameter to the constructor?

I think we should have a constructor that takes a HeapUsageProvider for unit testing this class.
Is that all you are after?
I struggle with shifting the knowledge of how to create an instance of HeapUsageProvider to the class that wants to create a HeapMemoryMonitor. The only class that needs to interact with the HeapUsageProvider is the HeapMemoryMonitor so it seems like it would be the class that should know how to create one. Or we need to introduce another class that is a HeapUsageProviderFactory. @kohlmu-pivotal Do you have any advice on what would be best?

Yes, I was asking why there is no constructor that takes a HeapUsageProvider.
Also, I think having a HeapUsageProviderFactory is a good idea. One could even make that Factory package-private. That will then help with "containing" it within a "context"

geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java

The biggest change is to no longer use the ServiceLoader. Instead users specificy their HeapUsageProvider class with a system property.

kohlmu-pivotal · 2022-03-23T23:32:36Z

geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java

+  private static HeapUsageProvider createHeapUsageProvider() {
+    Optional<String> propertyValue = getProductStringProperty(HEAP_USAGE_PROVIDER_CLASS_NAME);
+    if (propertyValue.isPresent()) {
+      String className = propertyValue.get();
+      Class<?> clazz;
+      try {
+        clazz = ClassPathLoader.getLatest().forName(className);
+      } catch (Exception ex) {
+        throw new IllegalArgumentException("Could not load class \"" + className +
+            "\" for system property \"geode." + HEAP_USAGE_PROVIDER_CLASS_NAME + "\"", ex);
+      }
+      Object instance;
+      try {
+        instance = clazz.newInstance();
+      } catch (Exception ex) {
+        throw new IllegalArgumentException("Could not create an instance of class \"" + className +
+            "\" for system property \"geode." + HEAP_USAGE_PROVIDER_CLASS_NAME +
+            "\". Make sure it has a public zero-arg constructor.", ex);
+      }
+      if (!(instance instanceof HeapUsageProvider)) {
+        throw new IllegalArgumentException("The class \"" + className +
+            "\" for system property \"geode." + HEAP_USAGE_PROVIDER_CLASS_NAME +
+            "\" is not an instance of HeapUsageProvider.");
+      }
+      return (HeapUsageProvider) instance;
+    } else {
+      return new MemoryPoolMXBeanHeapUsageProvider();


Two issues with this approach:

What happened to the idea of a Factory?

What happened to the idea of Service loading?

Generally we should avoid doing anything directly Classloader and reflection based work like this. Directly interacting with the Classloader just introduces unwarranted complexity. I cannot see a real benefit of doing this compared to ServiceLoader. But I can think of a few downsides.

Any chance this can be improved by creating a Factory and passing in the HeapUsageProvider into the HeapMemoryMonitor, at construction time?

The benefit is that ServiceLoader was not designed to load a singleton. If we used it then the user needs to not only implement the class but also define it as a service (one more file to edit) and we have to add some way for them to specify which of the services that implement HeapUsageProvider they want to use. So that ends up being a sys prop they need to define to signal the provider they want. It might also mean adding something like a "getName" to the interface. So these two solutions have much in common for the user (i.e. implement a class with a zero-arg constructor, get it on the class path, designate your class with a sys prop) but the service solution has the one additional thing to do (i.e. define it as a service). So from the user's point of view the service solution is more complex.
For the implementation geode already has the code to do this that it uses for other classes the user can plugin so it was very easy to implement and this code is well tested. I also liked that it was easy to see all the possible failure conditions and give an exception message that will give our users meaningful context about what went wrong.

This code could easily be moved into a factory

geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java

kohlmu-pivotal · 2022-03-24T00:38:27Z

geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java

-  public NotificationListener getHeapUsageMonitor() {
-    return (MemoryPoolMXBeanHeapUsageProvider) heapUsageMonitor;
+  public NotificationListener getHeapUsageProvider() {
+    return (MemoryPoolMXBeanHeapUsageProvider) heapUsageProvider;


Should this not be cast to a NotificationListener?

This method only exists to get an existing test: MemoryMonitorIntegrationTest to work.
But I think getHeapUsageProvider will go away in the near future. This level of detail should be tested by a unit test on MemoryPoolMXBeanHeapUsageProvider not this integration test.

kohlmu-pivotal · 2022-03-24T01:16:31Z

Overall this Draft PR proves that it is possible to have a pluggable implementation of a resource manager implementation depending on architecture and JDK. But overall I think there needs to be some work around the design of the solution. Going forward I expect some form of RFC to describe the new feature, it's impacts and design.

nabarunnag · 2022-03-24T22:40:40Z

geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java

@@ -332,12 +331,7 @@ public void setLowMemory(final ScheduledThreadPoolExecutor executor,
        final boolean isLowMemory,
        final long usedBytes,
        final InternalCache cache) {
-      if (cache.isQueryMonitorDisabledForLowMemory()) {


Will this be new behavior that we can set the low memory even if the query monitor is disabled?

I was trying to clean some of the dependencies the InternalResourceManager had with the InternalCache and thought it was safe to remove this call because I thought if it was disabled then getQueryMonitor would return null. But I was wrong. Even though it was disabled for low memory monitoring it could still exist because of MAX_QUERY_EXECUTION_TIME being > 0. And making this change broke ResourceManagerWithQueryMonitorDUnitTest. I just pushed a new revision that fixes this test but instead of always asking the cache if it is disabled, QueryMonitor is now told at construction time.

…rDUnitTest it is now fixed

added close method to HeapUsageProvider removed IllegalStateException from HeapUsageProvider

demery-pivotal reviewed Mar 21, 2022

View reviewed changes

dschneider-pivotal changed the title ~~initial refactoring introduces DefaultHeapUsageMonitor~~ make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI Mar 22, 2022

dschneider-pivotal force-pushed the HeapMonitorRefactor branch from de3c28a to 4a5ae0f Compare March 22, 2022 16:49

dschneider-pivotal added 2 commits March 22, 2022 10:27

preference is now given to using the pool specified with the HEAP_POO…

adde7b1

…L system property since "usageThreshold" is no longer used it is no longer required "collectionUsageThreshold" support is now optional

one more usageThreshold change to collectionUsageThreshold

875c3f1

Added Suppliers to MemoryPoolMXBeanHeapUsageProvider to make it unit …

ac79fb0

…testable

DonalEvans reviewed Mar 23, 2022

View reviewed changes

kohlmu-pivotal reviewed Mar 23, 2022

View reviewed changes

dschneider-pivotal added 2 commits March 23, 2022 14:50

did some cleanup based on review feedback

a3464d7

The biggest change is to no longer use the ServiceLoader. Instead users specificy their HeapUsageProvider class with a system property.

added log msg

e97f9a2

kohlmu-pivotal reviewed Mar 23, 2022

View reviewed changes

geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java Show resolved Hide resolved

kohlmu-pivotal reviewed Mar 24, 2022

View reviewed changes

nabarunnag reviewed Mar 24, 2022

View reviewed changes

dschneider-pivotal added 2 commits March 24, 2022 15:50

some changes in previous checkin broke ResourceManagerWithQueryMonito…

b89ddbc

…rDUnitTest it is now fixed

changed HeapUsageProvider sys prop name

0ef8081

added close method to HeapUsageProvider removed IllegalStateException from HeapUsageProvider

dschneider-pivotal closed this Jun 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI #7456

make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI #7456

dschneider-pivotal commented Mar 17, 2022 •

edited

demery-pivotal Mar 21, 2022

dschneider-pivotal Mar 21, 2022

upthewaterspout commented Mar 22, 2022

DonalEvans left a comment

DonalEvans Mar 22, 2022

DonalEvans Mar 22, 2022

dschneider-pivotal Mar 23, 2022

DonalEvans Mar 22, 2022

dschneider-pivotal commented Mar 23, 2022

kohlmu-pivotal Mar 23, 2022 •

edited

dschneider-pivotal Mar 23, 2022

dschneider-pivotal Mar 23, 2022

kohlmu-pivotal Mar 23, 2022

kohlmu-pivotal Mar 23, 2022

dschneider-pivotal Mar 23, 2022

kohlmu-pivotal Mar 23, 2022

kohlmu-pivotal Mar 23, 2022

dschneider-pivotal Mar 23, 2022

kohlmu-pivotal Mar 23, 2022

kohlmu-pivotal Mar 23, 2022

dschneider-pivotal Mar 24, 2022

dschneider-pivotal Mar 24, 2022

kohlmu-pivotal Mar 24, 2022

dschneider-pivotal Mar 24, 2022

kohlmu-pivotal commented Mar 24, 2022

nabarunnag Mar 24, 2022

dschneider-pivotal Mar 24, 2022

make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI #7456

make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI #7456

Conversation

dschneider-pivotal commented Mar 17, 2022 • edited

For all changes:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upthewaterspout commented Mar 22, 2022

DonalEvans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschneider-pivotal commented Mar 23, 2022

kohlmu-pivotal Mar 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohlmu-pivotal commented Mar 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschneider-pivotal commented Mar 17, 2022 •

edited

kohlmu-pivotal Mar 23, 2022 •

edited