[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7456: make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI
dschneider-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r834804743
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java
##
@@ -554,12 +579,12 @@ private String
createCriticalThresholdLogMessage(MemoryEvent event, String event
}
public long getMaxMemory() {
-return heapUsageMonitor.getMaxMemory();
+return heapUsageProvider.getMaxMemory();
}
@TestOnly
- public NotificationListener getHeapUsageMonitor() {
-return (MemoryPoolMXBeanHeapUsageProvider) heapUsageMonitor;
+ public NotificationListener getHeapUsageProvider() {
+return (MemoryPoolMXBeanHeapUsageProvider) heapUsageProvider;
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7456: make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI
dschneider-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r834800432
##
File path:
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()) {
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7456: make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI
dschneider-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r834745147
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java
##
@@ -88,30 +90,52 @@
@MutableForTesting
private static long testBytesUsedForThresholdSet = -1;
- @Immutable
- private static final ServiceLoader heapUsageMonitorLoader
=
- ServiceLoader.load(HeapUsageProvider.class);
-
HeapMemoryMonitor(final InternalResourceManager resourceManager, final
InternalCache cache,
final ResourceManagerStats stats) {
+this(resourceManager, cache, stats, createHeapUsageProvider());
+ }
+
+ @VisibleForTesting
+ HeapMemoryMonitor(final InternalResourceManager resourceManager, final
InternalCache cache,
+ final ResourceManagerStats stats, final HeapUsageProvider
heapUsageProvider) {
this.resourceManager = resourceManager;
resourceAdvisor = (ResourceAdvisor) cache.getDistributionAdvisor();
this.cache = cache;
this.stats = stats;
-heapUsageMonitor = findHeapUsageMonitor();
-thresholds = new MemoryThresholds(heapUsageMonitor.getMaxMemory());
+this.heapUsageProvider = heapUsageProvider;
+thresholds = new MemoryThresholds(heapUsageProvider.getMaxMemory());
mostRecentEvent = new MemoryEvent(ResourceType.HEAP_MEMORY,
MemoryState.DISABLED, MemoryState.DISABLED, null, 0L, true,
thresholds);
}
- private static HeapUsageProvider findHeapUsageMonitor() {
-for (HeapUsageProvider heapUsageMonitor : heapUsageMonitorLoader) {
- // return the first one defined as a service
- return heapUsageMonitor;
+ private static HeapUsageProvider createHeapUsageProvider() {
+Optional 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();
Review comment:
This code could easily be moved into a factory
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7456: make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI
dschneider-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r834744228
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java
##
@@ -88,30 +90,52 @@
@MutableForTesting
private static long testBytesUsedForThresholdSet = -1;
- @Immutable
- private static final ServiceLoader heapUsageMonitorLoader
=
- ServiceLoader.load(HeapUsageProvider.class);
-
HeapMemoryMonitor(final InternalResourceManager resourceManager, final
InternalCache cache,
final ResourceManagerStats stats) {
+this(resourceManager, cache, stats, createHeapUsageProvider());
+ }
+
+ @VisibleForTesting
+ HeapMemoryMonitor(final InternalResourceManager resourceManager, final
InternalCache cache,
+ final ResourceManagerStats stats, final HeapUsageProvider
heapUsageProvider) {
this.resourceManager = resourceManager;
resourceAdvisor = (ResourceAdvisor) cache.getDistributionAdvisor();
this.cache = cache;
this.stats = stats;
-heapUsageMonitor = findHeapUsageMonitor();
-thresholds = new MemoryThresholds(heapUsageMonitor.getMaxMemory());
+this.heapUsageProvider = heapUsageProvider;
+thresholds = new MemoryThresholds(heapUsageProvider.getMaxMemory());
mostRecentEvent = new MemoryEvent(ResourceType.HEAP_MEMORY,
MemoryState.DISABLED, MemoryState.DISABLED, null, 0L, true,
thresholds);
}
- private static HeapUsageProvider findHeapUsageMonitor() {
-for (HeapUsageProvider heapUsageMonitor : heapUsageMonitorLoader) {
- // return the first one defined as a service
- return heapUsageMonitor;
+ private static HeapUsageProvider createHeapUsageProvider() {
+Optional 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();
Review comment:
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7456: make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI
dschneider-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r834737637
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java
##
@@ -88,30 +90,52 @@
@MutableForTesting
private static long testBytesUsedForThresholdSet = -1;
- @Immutable
- private static final ServiceLoader heapUsageMonitorLoader
=
- ServiceLoader.load(HeapUsageProvider.class);
-
HeapMemoryMonitor(final InternalResourceManager resourceManager, final
InternalCache cache,
final ResourceManagerStats stats) {
+this(resourceManager, cache, stats, createHeapUsageProvider());
+ }
+
+ @VisibleForTesting
+ HeapMemoryMonitor(final InternalResourceManager resourceManager, final
InternalCache cache,
+ final ResourceManagerStats stats, final HeapUsageProvider
heapUsageProvider) {
this.resourceManager = resourceManager;
resourceAdvisor = (ResourceAdvisor) cache.getDistributionAdvisor();
this.cache = cache;
this.stats = stats;
-heapUsageMonitor = findHeapUsageMonitor();
-thresholds = new MemoryThresholds(heapUsageMonitor.getMaxMemory());
+this.heapUsageProvider = heapUsageProvider;
+thresholds = new MemoryThresholds(heapUsageProvider.getMaxMemory());
mostRecentEvent = new MemoryEvent(ResourceType.HEAP_MEMORY,
MemoryState.DISABLED, MemoryState.DISABLED, null, 0L, true,
thresholds);
}
- private static HeapUsageProvider findHeapUsageMonitor() {
-for (HeapUsageProvider heapUsageMonitor : heapUsageMonitorLoader) {
- // return the first one defined as a service
- return heapUsageMonitor;
+ private static HeapUsageProvider createHeapUsageProvider() {
+Optional propertyValue =
getProductStringProperty(HEAP_USAGE_PROVIDER_CLASS_NAME);
Review comment:
@kohlmu-pivotal I don't understand this comment. I think you are
questioning the name of the system property but it is
"geode.heapUsageProviderClassName" not "geode.classname". Since the value of
the property needs to be class that implements "HeapUsageProvider" I think the
proposed name is more helpful than "geode.resources.memory.provider". But I
think I might be missing your concern.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7456: make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI
dschneider-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r833602847
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java
##
@@ -144,82 +84,34 @@
private final ResourceAdvisor resourceAdvisor;
private final InternalCache cache;
private final ResourceManagerStats stats;
- private final TenuredHeapConsumptionMonitor tenuredHeapConsumptionMonitor;
- @MutableForTesting
- private static boolean testDisableMemoryUpdates = false;
@MutableForTesting
private static long testBytesUsedForThresholdSet = -1;
- /**
- * Determines if the name of the memory pool MXBean provided matches a list
of known tenured pool
- * names.
- *
- * Package private for testing.
- * checkTenuredHeapConsumption
- *
- * @param memoryPoolMXBean The memory pool MXBean to check.
- * @return True if the pool name matches a known tenured pool name, false
otherwise.
- */
- static boolean isTenured(MemoryPoolMXBean memoryPoolMXBean) {
-if (memoryPoolMXBean.getType() != MemoryType.HEAP) {
- return false;
-}
-
-String name = memoryPoolMXBean.getName();
-
-return name.equals("CMS Old Gen") // Sun Concurrent Mark Sweep GC
-|| name.equals("PS Old Gen") // Sun Parallel GC
-|| name.equals("G1 Old Gen") // Sun G1 GC
-|| name.equals("Old Space") // BEA JRockit 1.5, 1.6 GC
-|| name.equals("Tenured Gen") // Hitachi 1.5 GC
-|| name.equals("Java heap") // IBM 1.5, 1.6 GC
-|| name.equals("GenPauseless Old Gen") // azul C4/GPGC collector
-
-// Allow an unknown pool name to monitor
-|| (HEAP_POOL != null && name.equals(HEAP_POOL));
- }
+ @Immutable
+ private static final ServiceLoader heapUsageMonitorLoader
=
+ ServiceLoader.load(HeapUsageProvider.class);
HeapMemoryMonitor(final InternalResourceManager resourceManager, final
InternalCache cache,
- final ResourceManagerStats stats,
- TenuredHeapConsumptionMonitor tenuredHeapConsumptionMonitor) {
+ final ResourceManagerStats stats) {
Review comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7456: make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI
dschneider-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r833523572
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java
##
@@ -144,82 +84,34 @@
private final ResourceAdvisor resourceAdvisor;
private final InternalCache cache;
private final ResourceManagerStats stats;
- private final TenuredHeapConsumptionMonitor tenuredHeapConsumptionMonitor;
- @MutableForTesting
- private static boolean testDisableMemoryUpdates = false;
@MutableForTesting
private static long testBytesUsedForThresholdSet = -1;
- /**
- * Determines if the name of the memory pool MXBean provided matches a list
of known tenured pool
- * names.
- *
- * Package private for testing.
- * checkTenuredHeapConsumption
- *
- * @param memoryPoolMXBean The memory pool MXBean to check.
- * @return True if the pool name matches a known tenured pool name, false
otherwise.
- */
- static boolean isTenured(MemoryPoolMXBean memoryPoolMXBean) {
-if (memoryPoolMXBean.getType() != MemoryType.HEAP) {
- return false;
-}
-
-String name = memoryPoolMXBean.getName();
-
-return name.equals("CMS Old Gen") // Sun Concurrent Mark Sweep GC
-|| name.equals("PS Old Gen") // Sun Parallel GC
-|| name.equals("G1 Old Gen") // Sun G1 GC
-|| name.equals("Old Space") // BEA JRockit 1.5, 1.6 GC
-|| name.equals("Tenured Gen") // Hitachi 1.5 GC
-|| name.equals("Java heap") // IBM 1.5, 1.6 GC
-|| name.equals("GenPauseless Old Gen") // azul C4/GPGC collector
-
-// Allow an unknown pool name to monitor
-|| (HEAP_POOL != null && name.equals(HEAP_POOL));
- }
+ @Immutable
+ private static final ServiceLoader heapUsageMonitorLoader
=
+ ServiceLoader.load(HeapUsageProvider.class);
Review comment:
Good idea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7456: make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI
dschneider-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r833525265
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java
##
@@ -144,82 +84,34 @@
private final ResourceAdvisor resourceAdvisor;
private final InternalCache cache;
private final ResourceManagerStats stats;
- private final TenuredHeapConsumptionMonitor tenuredHeapConsumptionMonitor;
- @MutableForTesting
- private static boolean testDisableMemoryUpdates = false;
@MutableForTesting
private static long testBytesUsedForThresholdSet = -1;
- /**
- * Determines if the name of the memory pool MXBean provided matches a list
of known tenured pool
- * names.
- *
- * Package private for testing.
- * checkTenuredHeapConsumption
- *
- * @param memoryPoolMXBean The memory pool MXBean to check.
- * @return True if the pool name matches a known tenured pool name, false
otherwise.
- */
- static boolean isTenured(MemoryPoolMXBean memoryPoolMXBean) {
-if (memoryPoolMXBean.getType() != MemoryType.HEAP) {
- return false;
-}
-
-String name = memoryPoolMXBean.getName();
-
-return name.equals("CMS Old Gen") // Sun Concurrent Mark Sweep GC
-|| name.equals("PS Old Gen") // Sun Parallel GC
-|| name.equals("G1 Old Gen") // Sun G1 GC
-|| name.equals("Old Space") // BEA JRockit 1.5, 1.6 GC
-|| name.equals("Tenured Gen") // Hitachi 1.5 GC
-|| name.equals("Java heap") // IBM 1.5, 1.6 GC
-|| name.equals("GenPauseless Old Gen") // azul C4/GPGC collector
-
-// Allow an unknown pool name to monitor
-|| (HEAP_POOL != null && name.equals(HEAP_POOL));
- }
+ @Immutable
+ private static final ServiceLoader heapUsageMonitorLoader
=
+ ServiceLoader.load(HeapUsageProvider.class);
Review comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7456: make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI
dschneider-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r833522991
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java
##
@@ -144,82 +84,34 @@
private final ResourceAdvisor resourceAdvisor;
private final InternalCache cache;
private final ResourceManagerStats stats;
- private final TenuredHeapConsumptionMonitor tenuredHeapConsumptionMonitor;
- @MutableForTesting
- private static boolean testDisableMemoryUpdates = false;
@MutableForTesting
private static long testBytesUsedForThresholdSet = -1;
- /**
- * Determines if the name of the memory pool MXBean provided matches a list
of known tenured pool
- * names.
- *
- * Package private for testing.
- * checkTenuredHeapConsumption
- *
- * @param memoryPoolMXBean The memory pool MXBean to check.
- * @return True if the pool name matches a known tenured pool name, false
otherwise.
- */
- static boolean isTenured(MemoryPoolMXBean memoryPoolMXBean) {
-if (memoryPoolMXBean.getType() != MemoryType.HEAP) {
- return false;
-}
-
-String name = memoryPoolMXBean.getName();
-
-return name.equals("CMS Old Gen") // Sun Concurrent Mark Sweep GC
-|| name.equals("PS Old Gen") // Sun Parallel GC
-|| name.equals("G1 Old Gen") // Sun G1 GC
-|| name.equals("Old Space") // BEA JRockit 1.5, 1.6 GC
-|| name.equals("Tenured Gen") // Hitachi 1.5 GC
-|| name.equals("Java heap") // IBM 1.5, 1.6 GC
-|| name.equals("GenPauseless Old Gen") // azul C4/GPGC collector
-
-// Allow an unknown pool name to monitor
-|| (HEAP_POOL != null && name.equals(HEAP_POOL));
- }
+ @Immutable
+ private static final ServiceLoader heapUsageMonitorLoader
=
+ ServiceLoader.load(HeapUsageProvider.class);
HeapMemoryMonitor(final InternalResourceManager resourceManager, final
InternalCache cache,
- final ResourceManagerStats stats,
- TenuredHeapConsumptionMonitor tenuredHeapConsumptionMonitor) {
+ final ResourceManagerStats stats) {
this.resourceManager = resourceManager;
resourceAdvisor = (ResourceAdvisor) cache.getDistributionAdvisor();
this.cache = cache;
this.stats = stats;
-this.tenuredHeapConsumptionMonitor = tenuredHeapConsumptionMonitor;
+heapUsageMonitor = findHeapUsageMonitor();
+thresholds = new MemoryThresholds(heapUsageMonitor.getMaxMemory());
+mostRecentEvent = new MemoryEvent(ResourceType.HEAP_MEMORY,
+MemoryState.DISABLED, MemoryState.DISABLED, null, 0L, true,
thresholds);
}
- /**
- * Returns the tenured pool MXBean or throws an IllegaleStateException if
one couldn't be found.
- */
- public static MemoryPoolMXBean getTenuredMemoryPoolMXBean() {
-if (tenuredMemoryPoolMXBean != null) {
- return tenuredMemoryPoolMXBean;
+ private static HeapUsageProvider findHeapUsageMonitor() {
Review comment:
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"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7456: make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI
dschneider-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r833516193
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java
##
@@ -429,29 +207,33 @@ void setEvictionThreshold(final float evictionThreshold) {
thresholds = new MemoryThresholds(thresholds.getMaxMemoryBytes(),
thresholds.getCriticalThreshold(), evictionThreshold);
- updateStateAndSendEvent();
-
- // Start or stop monitoring based upon whether a threshold has been set
- if (thresholds.isEvictionThresholdEnabled()
- || thresholds.isCriticalThresholdEnabled()) {
-startMonitoring();
- } else if (!thresholds.isEvictionThresholdEnabled()
- && !thresholds.isCriticalThresholdEnabled()) {
-stopMonitoring();
- }
+ handleThresholdChange();
stats.changeEvictionThreshold(thresholds.getEvictionThresholdBytes());
}
}
+ private void handleThresholdChange() {
+updateStateAndSendEvent();
+
+// Start or stop monitoring based upon whether a threshold has been set
+if (thresholds.isEvictionThresholdEnabled()
+|| thresholds.isCriticalThresholdEnabled()) {
+ startMonitoring();
+} else if (!thresholds.isEvictionThresholdEnabled()
+&& !thresholds.isCriticalThresholdEnabled()) {
+ stopMonitoring();
+}
+ }
+
/**
* Compare the number of bytes used (fetched from the JVM) to the
thresholds. If necessary, change
* the state and send an event for the state change.
*/
public void updateStateAndSendEvent() {
updateStateAndSendEvent(
testBytesUsedForThresholdSet != -1 ? testBytesUsedForThresholdSet :
getBytesUsed(),
-"notification");
+"product-check");
Review comment:
I don't think so. All this String is used for is to put it in a log
message. If it was an ENUM then we would need to have each enum instance have a
nice printable string associated with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7456: make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI
dschneider-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r832734538
##
File path:
geode-core/src/main/java/org/apache/geode/cache/control/HeapUsageProvider.java
##
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.cache.control;
+
+import java.util.ServiceLoader;
+import java.util.function.LongConsumer;
+
+/**
+ * This interface can be implemented with a class
+ * that is then configured as a java service (see {@link ServiceLoader}
Review comment:
thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7456: make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI
dschneider-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r832734304
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/control/InternalResourceManager.java
##
@@ -66,11 +66,15 @@
public class InternalResourceManager implements ResourceManager {
private static final Logger logger = LogService.getLogger();
- 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);
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7456: make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI
dschneider-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r832733268
##
File path:
geode-core/src/integrationTest/java/org/apache/geode/internal/statistics/GemFireStatSamplerIntegrationTest.java
##
@@ -497,37 +492,6 @@ public void testArchiveRemoval() throws Exception {
waitForFileToDelete(archiveFile1, 10 * sampleRate, 10);
}
- @Test
- public void testLocalStatListenerRegistration() throws Exception {
Review comment:
The product no longer registers a stat listener for heap pool
monitoring. This test was testing it. This test already has other tests that
verify the stat listener feature in general (with no reference to the heap pool
monitoring). So the only coverage we are losing is for code that no longer
exists.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7456: make ResourceManager's interaction with JVM customizable with the HeapUsageProvider SPI
dschneider-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r832732061
##
File path:
geode-core/src/distributedTest/java/org/apache/geode/cache/management/MemoryThresholdsDUnitTest.java
##
@@ -1816,64 +1807,6 @@ public void fromData(DataInput in) throws IOException,
ClassNotFoundException {
}
}
- /**
- * putting this test here because junit does not have host stat sampler
enabled
- */
- @Test
- public void testLocalStatListenerRegistration() throws Exception {
Review comment:
No. It was testing something the product no longer does. The product
used to create either a stat listener or a poller thread (if stats were
disabled). Now it just consistently uses a poller thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
