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<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:
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
