DonalEvans commented on a change in pull request #7456: URL: https://github.com/apache/geode/pull/7456#discussion_r832709980
########## 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: There's a trailing `(` here. I think this should probably be `(see {@link ServiceLoader})` ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/control/MemoryPoolMXBeanHeapUsageProvider.java ########## @@ -0,0 +1,244 @@ +/* + * 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.internal.cache.control; + +import java.lang.management.MemoryPoolMXBean; +import java.lang.management.MemoryType; +import java.util.List; +import java.util.function.LongConsumer; +import java.util.function.LongSupplier; +import java.util.function.Supplier; + +import javax.management.ListenerNotFoundException; +import javax.management.Notification; +import javax.management.NotificationEmitter; +import javax.management.NotificationListener; + +import org.apache.logging.log4j.Logger; + +import org.apache.geode.annotations.Immutable; +import org.apache.geode.cache.control.HeapUsageProvider; +import org.apache.geode.logging.internal.log4j.api.LogService; +import org.apache.geode.util.internal.GeodeGlossary; + +/** + * This heap usage provider is used by default. + * It determines heap usage by using a MemoryPoolMXBean that it gets from the JVM + * using the ManagementFactory. + */ +public class MemoryPoolMXBeanHeapUsageProvider implements HeapUsageProvider, NotificationListener { + private static final Logger logger = LogService.getLogger(); + // Allow for an unknown heap pool for VMs we may support in the future. + private static final String HEAP_POOL = + System.getProperty(GeodeGlossary.GEMFIRE_PREFIX + "ResourceManager.HEAP_POOL"); + @Immutable + private static final LongConsumer disabledHeapUsageListener = (usedMemory) -> { + }; + + private static MemoryPoolMXBean findTenuredMemoryPoolMXBean( + Supplier<List<MemoryPoolMXBean>> memoryPoolSupplier) { + if (HEAP_POOL != null && !HEAP_POOL.equals("")) { + // give preference to using an existing pool that matches HEAP_POOL + for (MemoryPoolMXBean memoryPoolMXBean : memoryPoolSupplier.get()) { + if (HEAP_POOL.equals(memoryPoolMXBean.getName())) { + return memoryPoolMXBean; + } + } + logger.warn("No memory pool was found with the name {}. Known pools are: {}", + HEAP_POOL, getAllMemoryPoolNames(memoryPoolSupplier)); + + } + for (MemoryPoolMXBean memoryPoolMXBean : memoryPoolSupplier.get()) { + if (isTenured(memoryPoolMXBean)) { + if (HEAP_POOL != null && !HEAP_POOL.equals("")) { + logger.warn( + "Ignoring gemfire.ResourceManager.HEAP_POOL system property and using pool {}.", + memoryPoolMXBean.getName()); + } + return memoryPoolMXBean; + } + } + logger.error("No tenured pools found. Known pools are: {}", + getAllMemoryPoolNames(memoryPoolSupplier)); + return null; + } + + /* + * Calculates the max memory for the tenured pool. Works around JDK bug: + * http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7078465 by getting max memory from runtime + * and subtracting all other heap pools from it. + */ + private static long calculateTenuredPoolMaxMemory( + Supplier<List<MemoryPoolMXBean>> memoryPoolSupplier, + LongSupplier maxJVMHeapSupplier, + MemoryPoolMXBean poolBean) { + if (poolBean != null && poolBean.getUsage().getMax() != -1) { + return poolBean.getUsage().getMax(); + } else { + long calculatedMaxMemory = maxJVMHeapSupplier.getAsLong(); + for (MemoryPoolMXBean p : memoryPoolSupplier.get()) { + if (p.getType() == MemoryType.HEAP && p.getUsage().getMax() != -1) { + calculatedMaxMemory -= p.getUsage().getMax(); + } + } + return calculatedMaxMemory; + } + } + + /** + * Determines if the name of the memory pool MXBean provided matches a list of known tenured pool + * names. + * + * @param memoryPoolMXBean The memory pool MXBean to check. + * @return True if the pool name matches a known tenured pool name, false otherwise. + */ + private 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 + || name.equals("ZHeap") // ZGC + ; + } + + /** + * Returns the names of all available memory pools as a single string. + */ + private static String getAllMemoryPoolNames(Supplier<List<MemoryPoolMXBean>> memoryPoolSupplier) { + StringBuilder builder = new StringBuilder("["); + + for (MemoryPoolMXBean memoryPoolBean : memoryPoolSupplier.get()) { + builder.append("(Name=").append(memoryPoolBean.getName()).append(";Type=") + .append(memoryPoolBean.getType()).append(";collectionUsageThresholdSupported=") + .append(memoryPoolBean.isCollectionUsageThresholdSupported()).append("), "); + } + + if (builder.length() > 1) { + builder.setLength(builder.length() - 2); + } + builder.append("]"); + + return builder.toString(); + } + + 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; Review comment: 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. ########## 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: Are we losing coverage with this test being removed? Would it be possible to write an equivalent test for the new implementation? ########## 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: Are we losing coverage with this test being removed? Would it be possible to write an equivalent test for the new implementation? ########## File path: geode-core/src/integrationTest/java/org/apache/geode/internal/cache/control/MemoryMonitorIntegrationTest.java ########## @@ -380,37 +379,28 @@ public void testCriticalHeapThreshold() throws Exception { final float justright = 92.5f; final ResourceManager rm = cache.getResourceManager(); - long usageThreshold = -1; - int once = 0; - for (MemoryPoolMXBean p : ManagementFactory.getMemoryPoolMXBeans()) { - if (p.isUsageThresholdSupported() && HeapMemoryMonitor.isTenured(p)) { - usageThreshold = p.getUsageThreshold(); - once++; - } - } - assertEquals("Expected only one pool to be assigned", 1, once); - // Default test, current default is disabled assertEquals(rm.getCriticalHeapPercentage(), MemoryThresholds.DEFAULT_CRITICAL_PERCENTAGE, 0.01); NotificationEmitter emitter = (NotificationEmitter) ManagementFactory.getMemoryMXBean(); 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) { } Review comment: 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. ########## 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: Why is this default value being increased? -- 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
