kohlmu-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r832778862
##########
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<HeapUsageProvider> heapUsageMonitorLoader
=
+ ServiceLoader.load(HeapUsageProvider.class);
HeapMemoryMonitor(final InternalResourceManager resourceManager, final
InternalCache cache,
- final ResourceManagerStats stats,
- TenuredHeapConsumptionMonitor tenuredHeapConsumptionMonitor) {
+ final ResourceManagerStats stats) {
Review comment:
Why would we not pass in the correct HeapUsageProvider as a parameter to
the constructor?
##########
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:
Would it not better served to have an ENUM of eventOrigins?
##########
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<HeapUsageProvider> 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:
Could we possibly change this to a "getHeapUsageMonitor"... It is not
really a "find" method
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java
##########
@@ -754,33 +481,11 @@ protected HeapMemoryMonitor setMostRecentEvent(
return this;
}
- class LocalHeapStatListener implements LocalStatListener {
- /*
- * (non-Javadoc)
- *
- * @see
org.apache.geode.internal.statistics.LocalStatListener#statValueChanged(double)
- */
- @Override
- @SuppressWarnings("synthetic-access")
- public void statValueChanged(double value) {
- final long usedBytes = (long) value;
- try {
- resourceManager.runWithNotifyExecutor(() -> {
- if (!testDisableMemoryUpdates) {
- updateStateAndSendEvent(usedBytes, "polling");
- }
- });
- if (HeapMemoryMonitor.logger.isDebugEnabled()) {
- HeapMemoryMonitor.logger.debug(
- "StatSampler scheduled a " + "handleNotification call with " +
usedBytes + " bytes");
- }
- } catch (RejectedExecutionException ignore) {
- if (!resourceManager.isClosed()) {
- logger.warn("No memory events will be delivered because of
RejectedExecutionException");
- }
- } catch (CacheClosedException ignore) {
- // nothing to do
- }
+ @Override
+ public void accept(long usedMemory) {
+ if (!testDisableMemoryUpdates) {
+ resourceManager
+ .runWithNotifyExecutor(() -> updateStateAndSendEvent(usedMemory,
"notification"));
Review comment:
Same as previously.. Would it not make more sense to have an ENUM as an
eventOrigin?
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/control/HeapMemoryMonitor.java
##########
@@ -860,4 +552,52 @@ private String
createCriticalThresholdLogMessage(MemoryEvent event, String event
+ " Used bytes: " + event.getBytesUsed() + "."
+ " Memory thresholds: " + thresholds;
}
+
+ public long getMaxMemory() {
+ return heapUsageMonitor.getMaxMemory();
+ }
+
+ @TestOnly
+ public NotificationListener getHeapUsageMonitor() {
+ return (MemoryPoolMXBeanHeapUsageProvider) heapUsageMonitor;
+ }
+
+ private ScheduledFuture<?> heapUsagePollerFuture;
+
+ /**
+ * Start a separate thread for polling the JVM for heap memory usage.
+ */
+ private void startHeapUsagePoller() {
+ heapUsagePollerFuture =
resourceManager.getExecutor().scheduleAtFixedRate(new HeapUsagePoller(),
+ POLLER_INTERVAL, POLLER_INTERVAL,
+ TimeUnit.MILLISECONDS);
+
+ if (logger.isDebugEnabled()) {
+ logger.debug(
+ "Started GemfireHeapPoller to poll the heap every " +
POLLER_INTERVAL + " milliseconds");
+ }
+ }
+
+ private void stopHeapUsagePoller() {
+ ScheduledFuture<?> future = heapUsagePollerFuture;
+ if (future != null) {
+ future.cancel(false);
+ heapUsagePollerFuture = null;
+ }
+ }
+
+ /**
+ * Polls the heap usage
+ */
+ private class HeapUsagePoller implements Runnable {
+ @Override
+ public void run() {
+ try {
+ updateStateAndSendEvent(heapUsageMonitor.getBytesUsed(), "polling");
Review comment:
Could the eventOrigin not be an ENUM?
--
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
