apache / accumulo

Apache Accumulo
https://accumulo.apache.org
Apache License 2.0
1.08k stars 445 forks source link

NullPointerException thrown in monitor when visiting the External Compactions page #5098

Closed DomGarguilo closed 1 day ago

DomGarguilo commented 6 days ago

Describe the bug When requesting external compaction data from the monitor server, the following code (added in ) is used to memoize and return that data: https://github.com/apache/accumulo/blob/767a68ada50131317e3420dc14780505c523789f/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java#L686-L711

This code was added in #4867.

Versions (OS, Maven, Java, and others, as appropriate):

To Reproduce Steps to reproduce the behavior (or a link to an example repository that reproduces the problem):

  1. Start an instance of accumulo
  2. Go to the external compactions page
  3. look at the monitors logs

Here are some portions of the logs:

2024-11-22T15:21:46,486 [monitor.Monitor] INFO : User initiated fetch of running External Compactions from localhost:9132
2024-11-22T15:21:46,486 [external.ECResource] INFO : Got coordinator from monitor = Optional[localhost:9132]
2024-11-22T15:21:46,487 [server.HttpChannel] WARN : /rest/ec/running
jakarta.servlet.ServletException: java.lang.NullPointerException: Cannot invoke "Object.getClass()" because "m" is null
    at org.glassfish.jersey.servlet.WebComponent.serviceImpl(WebComponent.java:410) ~[jersey-container-servlet-core-3.0.9.jar:?]
    at org.glassfish.jersey.servlet.WebComponent.service(WebComponent.java:346) ~[jersey-container-servlet-core-3.0.9.jar:?]
    at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:358) ~[jersey-container-servlet-core-3.0.9.jar:?]
    at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:311) ~[jersey-container-servlet-core-3.0.9.jar:?]
...
Caused by: java.lang.NullPointerException: Cannot invoke "Object.getClass()" because "m" is null
    at java.base/java.util.Collections.unmodifiableMap(Collections.java:1476) ~[?:?]
    at org.apache.accumulo.monitor.Monitor$ExternalCompactionsSnapshot.<init>(Monitor.java:691) ~[accumulo-monitor-2.1.4-SNAPSHOT.jar:2.1.4-SNAPSHOT]
    at org.apache.accumulo.monitor.Monitor.computeExternalCompactionsSnapshot(Monitor.java:710) ~[accumulo-monitor-2.1.4-SNAPSHOT.jar:2.1.4-SNAPSHOT]
    at org.apache.accumulo.monitor.Monitor.lambda$new$1(Monitor.java:635) ~[accumulo-monitor-2.1.4-SNAPSHOT.jar:2.1.4-SNAPSHOT]
    at com.google.common.base.Suppliers$ExpiringMemoizingSupplier.get(Suppliers.java:261) ~[guava-33.0.0-jre.jar:?]
    at org.apache.accumulo.monitor.Monitor.getRunnningCompactions(Monitor.java:714) ~[accumulo-monitor-2.1.4-SNAPSHOT.jar:2.1.4-SNAPSHOT]
    at org.apache.accumulo.monitor.rest.compactions.external.ECResource.getRunning(ECResource.java:63) ~[accumulo-monitor-2.1.4-SNAPSHOT.jar:2.1.4-SNAPSHOT]
...
DomGarguilo commented 6 days ago

The following makes it so the exception is not thrown but I'm not sure if this is the best solution:

diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java
index 80dd69b8cd..bd5111af47 100644
--- a/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java
+++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java
@@ -688,7 +688,7 @@ public class Monitor extends AbstractServer implements HighlyAvailableService {
     public final Map<String,TExternalCompaction> ecRunningMap;

     private ExternalCompactionsSnapshot(Map<String,TExternalCompaction> ecRunningMap) {
-      this.ecRunningMap = Collections.unmodifiableMap(ecRunningMap);
+      this.ecRunningMap = ecRunningMap == null ? Collections.emptyMap() : Collections.unmodifiableMap(ecRunningMap);
       this.runningCompactions = new RunningCompactions(ecRunningMap);
     }
   }
cshannon commented 6 days ago

Looking at the code, that map having a null is valid so I think simply checking for null like the above example (or maybe using Optional) and just setting it to an immutable map should be fine in this case.

dlmarion commented 1 day ago

@DomGarguilo - can this be closed, or is there more to do?