scylla-jmx: Use registration checker objects

Fixes #134
Refs #135

Replaces previous refresh calls with ones bound to registration
check objects, which provides some sync between threads doing
refresh, and reduced redundant calls.

Also adds repeated reaping of dead objects, i.e. every 5 minutes
we try to remove dead CF:s (not adding new ones), to reduce
idle footprint.

src/main/java/com/scylladb/jmx/metrics/APIMBean.java

@@ -1,6 +1,7 @@
 package com.scylladb.jmx.metrics;
 
 import java.lang.reflect.Field;
+import java.util.EnumSet;
 import java.util.Set;
 import java.util.function.Function;
 import java.util.function.Predicate;
@@ -55,35 +56,39 @@ public class APIMBean implements MBeanRegistration {
      * @param generator
      *            {@link Function} to create a new MBean instance for a given
      *            {@link ObjectName}
-     * 
      * @return
      * @throws MalformedObjectNameException
      */
-    public static boolean checkRegistration(JmxMBeanServer server, Set<ObjectName> all,
+	public static boolean checkRegistration(JmxMBeanServer server, Set<ObjectName> all,
-            final Predicate<ObjectName> predicate, Function<ObjectName, Object> generator)
+			EnumSet<RegistrationMode> mode, final Predicate<ObjectName> predicate,
-            throws MalformedObjectNameException {
+			Function<ObjectName, Object> generator) throws MalformedObjectNameException {
-        Set<ObjectName> registered = queryNames(server, predicate);
+		Set<ObjectName> registered = queryNames(server, predicate);
-        for (ObjectName name : registered) {
+		if (mode.contains(RegistrationMode.Remove)) {
-            if (!all.contains(name)) {
+			for (ObjectName name : registered) {
-                try {
+				if (!all.contains(name)) {
-                    server.getMBeanServerInterceptor().unregisterMBean(name);
+					try {
-                } catch (MBeanRegistrationException | InstanceNotFoundException e) {
+						server.getMBeanServerInterceptor().unregisterMBean(name);
-                }
+					} catch (MBeanRegistrationException | InstanceNotFoundException e) {
-            }
+					}
-        }
+				}
+			}
+		}
 
-        int added = 0;
+		int added = 0;
-        for (ObjectName name : all) {
+		if (mode.contains(RegistrationMode.Add)) {
-            if (!registered.contains(name)) {
+			for (ObjectName name : all) {
-                try {
+				if (!registered.contains(name)) {
-                    server.getMBeanServerInterceptor().registerMBean(generator.apply(name), name);
+					try {
-                    added++;
+						server.getMBeanServerInterceptor().registerMBean(generator.apply(name), name);
-                } catch (InstanceAlreadyExistsException | MBeanRegistrationException | NotCompliantMBeanException e) {
+						added++;
-                }
+					} catch (InstanceAlreadyExistsException | MBeanRegistrationException
-            }
+							| NotCompliantMBeanException e) {
-        }
+					}
-        return added > 0;
+				}
-    }
+			}
+		}
+		return added > 0;
+	}
 
     /**
      * Helper method to query {@link ObjectName}s from an {@link MBeanServer}

src/main/java/com/scylladb/jmx/utils/APIMBeanServer.java

@@ -1,8 +1,13 @@
 package com.scylladb.jmx.utils;
 
+import static java.util.Arrays.asList;
+import static java.util.concurrent.Executors.newScheduledThreadPool;
+import static java.util.concurrent.TimeUnit.MINUTES;
+
 import java.io.ObjectInputStream;
 import java.net.UnknownHostException;
 import java.util.Set;
+import java.util.concurrent.ScheduledExecutorService;
 import java.util.logging.Logger;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
@@ -34,12 +39,17 @@ import org.apache.cassandra.db.ColumnFamilyStore;
 import org.apache.cassandra.metrics.StreamingMetrics;
 
 import com.scylladb.jmx.api.APIClient;
+import com.scylladb.jmx.metrics.RegistrationChecker;
 import com.sun.jmx.mbeanserver.JmxMBeanServer;
 
 @SuppressWarnings("restriction")
 public class APIMBeanServer implements MBeanServer {
     @SuppressWarnings("unused")
     private static final Logger logger = Logger.getLogger(APIMBeanServer.class.getName());
+    private static final ScheduledExecutorService executor = newScheduledThreadPool(1);
+
+    private final RegistrationChecker columnFamilyStoreChecker = ColumnFamilyStore.createRegistrationChecker();
+    private final RegistrationChecker streamingMetricsChecker = StreamingMetrics.createRegistrationChecker();
 
     private final APIClient client;
     private final JmxMBeanServer server;
@@ -47,6 +57,16 @@ public class APIMBeanServer implements MBeanServer {
     public APIMBeanServer(APIClient client, JmxMBeanServer server) {
         this.client = client;
         this.server = server;
+        
+    	executor.scheduleWithFixedDelay(() -> {
+    		for (RegistrationChecker c : asList(columnFamilyStoreChecker, streamingMetricsChecker)) {
+    			try {
+    				c.reap(client, server);
+    	        } catch (OperationsException | UnknownHostException e) {
+    	            // TODO: log?
+    	        }
+    		}
+        }, 1, 5, MINUTES);
     }
 
     private static ObjectInstance prepareForRemote(final ObjectInstance i) {
@@ -65,7 +85,7 @@ public class APIMBeanServer implements MBeanServer {
             throw new IllegalArgumentException(n.toString());
         }
     }
-
+    
     @Override
     public ObjectInstance createMBean(String className, ObjectName name) throws ReflectionException,
             InstanceAlreadyExistsException, MBeanRegistrationException, MBeanException, NotCompliantMBeanException {
@@ -286,25 +306,22 @@ public class APIMBeanServer implements MBeanServer {
     }
 
     static final Pattern tables = Pattern.compile("^\\*?((Index)?ColumnFamil(ies|y)|(Index)?(Table(s)?)?)$");
-
+    
-    private boolean checkRegistrations(ObjectName name) {
+    private void checkRegistrations(ObjectName name) {
         if (name != null && server.isRegistered(name)) {
-            return false;
+            return;
         }
-        
+                
-        boolean result = false;
-        
         try {
             String type = name != null ? name.getKeyProperty("type") : null;            
             if (type == null || tables.matcher(type).matches()) {
-                result |= ColumnFamilyStore.checkRegistration(client, server);
+            	columnFamilyStoreChecker.check(client, server);
             }
             if (type == null || StreamingMetrics.TYPE_NAME.equals(type)) {
-                result |= StreamingMetrics.checkRegistration(client, server);
+            	streamingMetricsChecker.check(client, server);
             }
-        } catch (MalformedObjectNameException | UnknownHostException e) {
+        } catch (OperationsException | UnknownHostException e) {
             // TODO: log
         }
-        return result;
     }
 }

src/main/java/org/apache/cassandra/db/ColumnFamilyStore.java

@@ -30,6 +30,7 @@ import static javax.json.Json.createObjectBuilder;
 
 import java.io.StringReader;
 import java.util.Collections;
+import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -50,6 +51,7 @@ import javax.json.JsonReader;
 import javax.management.MBeanServer;
 import javax.management.MalformedObjectNameException;
 import javax.management.ObjectName;
+import javax.management.OperationsException;
 import javax.management.openmbean.CompositeData;
 import javax.management.openmbean.CompositeDataSupport;
 import javax.management.openmbean.CompositeType;
@@ -65,6 +67,8 @@ import org.apache.cassandra.metrics.TableMetrics;
 
 import com.scylladb.jmx.api.APIClient;
 import com.scylladb.jmx.metrics.MetricsMBean;
+import com.scylladb.jmx.metrics.RegistrationChecker;
+import com.scylladb.jmx.metrics.RegistrationMode;
 import com.sun.jmx.mbeanserver.JmxMBeanServer;
 import com.google.common.base.Throwables;
 
@@ -182,15 +186,22 @@ public class ColumnFamilyStore extends MetricsMBean implements ColumnFamilyStore
                 "org.apache.cassandra.db:type=" + type + ",keyspace=" + keyspace + ",columnfamily=" + name);
     }
 
-    public static boolean checkRegistration(APIClient client, JmxMBeanServer server) throws MalformedObjectNameException {
+	public static RegistrationChecker createRegistrationChecker() {
-        JsonArray mbeans = client.getJsonArray("/column_family/");
+		return new RegistrationChecker() {
-        Set<ObjectName> all = new HashSet<ObjectName>();
+			@Override
-        for (int i = 0; i < mbeans.size(); i++) {
+			protected void doCheck(APIClient client, JmxMBeanServer server, EnumSet<RegistrationMode> mode)
-            JsonObject mbean = mbeans.getJsonObject(i);
+					throws OperationsException {
-            all.add(getName(mbean.getString("type"), mbean.getString("ks"), mbean.getString("cf")));
+				JsonArray mbeans = client.getJsonArray("/column_family/");
-        }
+				Set<ObjectName> all = new HashSet<ObjectName>();
-        return checkRegistration(server, all, n -> TYPE_NAMES.contains(n.getKeyProperty("type")), n -> new ColumnFamilyStore(client, n));
+				for (int i = 0; i < mbeans.size(); i++) {
-    }
+					JsonObject mbean = mbeans.getJsonObject(i);
+					all.add(getName(mbean.getString("type"), mbean.getString("ks"), mbean.getString("cf")));
+				}
+				checkRegistration(server, all, mode,
+						n -> TYPE_NAMES.contains(n.getKeyProperty("type")), n -> new ColumnFamilyStore(client, n));
+			}
+		};
+	}
 
     /**
      * @return the name of the column family

src/main/java/org/apache/cassandra/metrics/StreamingMetrics.java

@@ -24,20 +24,23 @@
 package org.apache.cassandra.metrics;
 
 import static java.util.Arrays.asList;
-import static java.util.Collections.emptySet;
 import static org.apache.cassandra.metrics.DefaultNameFactory.createMetricName;
 
 import java.net.InetAddress;
 import java.net.UnknownHostException;
+import java.util.EnumSet;
 import java.util.HashSet;
 import java.util.Set;
 
 import javax.json.JsonArray;
 import javax.management.MalformedObjectNameException;
 import javax.management.ObjectName;
+import javax.management.OperationsException;
 
 import com.scylladb.jmx.api.APIClient;
 import com.scylladb.jmx.metrics.APIMBean;
+import com.scylladb.jmx.metrics.RegistrationChecker;
+import com.scylladb.jmx.metrics.RegistrationMode;
 import com.sun.jmx.mbeanserver.JmxMBeanServer;
 
 /**
@@ -64,46 +67,45 @@ public class StreamingMetrics {
     private static boolean isStreamingName(ObjectName n) {
         return TYPE_NAME.equals(n.getKeyProperty("type"));
     }
+    
+	public static RegistrationChecker createRegistrationChecker() {
+		return new RegistrationChecker() {			
+			@Override
+			protected void doCheck(APIClient client, JmxMBeanServer server, EnumSet<RegistrationMode> mode) throws OperationsException, UnknownHostException {
+				Set<ObjectName> all = new HashSet<ObjectName>(globalNames);
+				JsonArray streams = client.getJsonArray("/stream_manager/");
+				for (int i = 0; i < streams.size(); i++) {
+					JsonArray sessions = streams.getJsonObject(i).getJsonArray("sessions");
+					for (int j = 0; j < sessions.size(); j++) {
+						String peer = sessions.getJsonObject(j).getString("peer");
+						String scope = InetAddress.getByName(peer).getHostAddress().replaceAll(":", ".");
+						all.add(createMetricName(TYPE_NAME, "IncomingBytes", scope));
+						all.add(createMetricName(TYPE_NAME, "OutgoingBytes", scope));
+					}
+				}
 
-    public static void unregister(APIClient client, JmxMBeanServer server) throws MalformedObjectNameException {
+				MetricsRegistry registry = new MetricsRegistry(client, server);
-        APIMBean.checkRegistration(server, emptySet(), StreamingMetrics::isStreamingName, (n) -> null);
+				APIMBean.checkRegistration(server, all, mode, StreamingMetrics::isStreamingName, n -> {
-    }
+					String scope = n.getKeyProperty("scope");
+					String name = n.getKeyProperty("name");
 
-    public static boolean checkRegistration(APIClient client, JmxMBeanServer server)
+					String url = null;
-            throws MalformedObjectNameException, UnknownHostException {
+					if ("ActiveOutboundStreams".equals(name)) {
-
+						url = "/stream_manager/metrics/outbound";
-        Set<ObjectName> all = new HashSet<ObjectName>(globalNames);
+					} else if ("IncomingBytes".equals(name) || "TotalIncomingBytes".equals(name)) {
-        JsonArray streams = client.getJsonArray("/stream_manager/");
+						url = "/stream_manager/metrics/incoming";
-        for (int i = 0; i < streams.size(); i++) {
+					} else if ("OutgoingBytes".equals(name) || "TotalOutgoingBytes".equals(name)) {
-            JsonArray sessions = streams.getJsonObject(i).getJsonArray("sessions");
+						url = "/stream_manager/metrics/outgoing";
-            for (int j = 0; j < sessions.size(); j++) {
+					}
-                String peer = sessions.getJsonObject(j).getString("peer");
+					if (url == null) {
-                String scope = InetAddress.getByName(peer).getHostAddress().replaceAll(":", ".");
+						throw new IllegalArgumentException();
-                all.add(createMetricName(TYPE_NAME, "IncomingBytes", scope));
+					}
-                all.add(createMetricName(TYPE_NAME, "OutgoingBytes", scope));
+					if (scope != null) {
-            }
+						url = url + "/" + scope;
-        }
+					}
-
+					return registry.counter(url);
-        MetricsRegistry registry = new MetricsRegistry(client, server);
+				});
-        return APIMBean.checkRegistration(server, all, StreamingMetrics::isStreamingName, n -> {
+			}
-            String scope = n.getKeyProperty("scope");
+		};
-            String name = n.getKeyProperty("name");
+	}
-
-            String url = null;
-            if ("ActiveOutboundStreams".equals(name)) {
-                url = "/stream_manager/metrics/outbound";
-            } else if ("IncomingBytes".equals(name) || "TotalIncomingBytes".equals(name)) {
-                url = "/stream_manager/metrics/incoming";
-            } else if ("OutgoingBytes".equals(name) || "TotalOutgoingBytes".equals(name)) {
-                url = "/stream_manager/metrics/outgoing";
-            }
-            if (url == null) {
-                throw new IllegalArgumentException();
-            }
-            if (scope != null) {
-                url = url + "/" + scope;
-            }
-            return registry.counter(url);
-        });
-    }
 }