kohlmu-pivotal commented on a change in pull request #7456:
URL: https://github.com/apache/geode/pull/7456#discussion_r833801795
##########
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<HeapUsageProvider> 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<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();
Review comment:
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?
--
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
