From 2c48bab91a5bd067f68a8a19da44c964553ff9b3 Mon Sep 17 00:00:00 2001 From: Piotr Jastrzebski Date: Sat, 12 May 2018 18:35:18 +0200 Subject: [PATCH 1/3] Use JmxMBeanServer instead of MBeanServer JmxMBeanServer is a concrete implementation of a MBeanServer. We want to use it directly because we need to bypass calls to JmxMBeanServer.registerMBean and JmxMBeanServer.unregisterMBean. They take ObjectName as parameter, copy it using ObjectName.getInstance(ObjectName) and pass it to registerMBean and unregisterMBean of JmxMBeanServer.getMBeanServerInterceptor(). We want to avoid this copy and pass the ObjectName argument directly to JmxMBeanServer.getMBeanServerInterceptor(). To do that this patch: 1. changes all MBeanServer variables to JmxMBeanServer 2. creates JmxMBeanServer in APIBuilder making sure accessing interceptors is allowed 3. makes sure that JmxMBeanServer.getMBeanServerInterceptor().registerMBean is called wherever JmxMBeanServer.registerMBean was called 4. makes sure that JmxMBeanServer.getMBeanServerInterceptor().unregisterMBean is called whenever JmxMBeanServer.unregisterMBean was called Next patch will use different ObjectName implementation that will use less memory and this patch is crucial because without it every ObjectName is transformed with ObjectName.getInstance which turns the object into a regular ObjectName. Signed-off-by: Piotr Jastrzebski --- .../java/com/scylladb/jmx/metrics/APIMBean.java | 13 +++++++------ .../java/com/scylladb/jmx/metrics/MetricsMBean.java | 7 ++++--- .../java/com/scylladb/jmx/utils/APIBuilder.java | 6 +++++- .../java/com/scylladb/jmx/utils/APIMBeanServer.java | 5 +++-- .../org/apache/cassandra/db/ColumnFamilyStore.java | 3 ++- .../apache/cassandra/metrics/MetricsRegistry.java | 7 ++++--- .../apache/cassandra/metrics/StreamingMetrics.java | 6 +++--- 7 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/main/java/com/scylladb/jmx/metrics/APIMBean.java b/src/main/java/com/scylladb/jmx/metrics/APIMBean.java index 4c57171..8eb8a01 100644 --- a/src/main/java/com/scylladb/jmx/metrics/APIMBean.java +++ b/src/main/java/com/scylladb/jmx/metrics/APIMBean.java @@ -20,6 +20,7 @@ import javax.management.ObjectName; import javax.management.QueryExp; import com.scylladb.jmx.api.APIClient; +import com.sun.jmx.mbeanserver.JmxMBeanServer; /** * Base type for MBeans in scylla-jmx. Wraps auto naming and {@link APIClient} @@ -58,14 +59,14 @@ public class APIMBean implements MBeanRegistration { * @return * @throws MalformedObjectNameException */ - public static boolean checkRegistration(MBeanServer server, Set all, + public static boolean checkRegistration(JmxMBeanServer server, Set all, final Predicate predicate, Function generator) throws MalformedObjectNameException { Set registered = queryNames(server, predicate); for (ObjectName name : registered) { if (!all.contains(name)) { try { - server.unregisterMBean(name); + server.getMBeanServerInterceptor().unregisterMBean(name); } catch (MBeanRegistrationException | InstanceNotFoundException e) { } } @@ -75,7 +76,7 @@ public class APIMBean implements MBeanRegistration { for (ObjectName name : all) { if (!registered.contains(name)) { try { - server.registerMBean(generator.apply(name), name); + server.getMBeanServerInterceptor().registerMBean(generator.apply(name), name); added++; } catch (InstanceAlreadyExistsException | MBeanRegistrationException | NotCompliantMBeanException e) { } @@ -92,7 +93,7 @@ public class APIMBean implements MBeanRegistration { * @param predicate * @return */ - public static Set queryNames(MBeanServer server, final Predicate predicate) { + public static Set queryNames(JmxMBeanServer server, final Predicate predicate) { @SuppressWarnings("serial") Set registered = server.queryNames(null, new QueryExp() { @Override @@ -108,7 +109,7 @@ public class APIMBean implements MBeanRegistration { return registered; } - MBeanServer server; + JmxMBeanServer server; ObjectName name; protected final ObjectName getBoundName() { @@ -162,7 +163,7 @@ public class APIMBean implements MBeanRegistration { if (this.server != null) { throw new IllegalStateException("Can only exist in a single MBeanServer"); } - this.server = server; + this.server = (JmxMBeanServer) server; if (name == null) { name = generateName(); } diff --git a/src/main/java/com/scylladb/jmx/metrics/MetricsMBean.java b/src/main/java/com/scylladb/jmx/metrics/MetricsMBean.java index ae4a87f..90aada6 100644 --- a/src/main/java/com/scylladb/jmx/metrics/MetricsMBean.java +++ b/src/main/java/com/scylladb/jmx/metrics/MetricsMBean.java @@ -16,6 +16,7 @@ import org.apache.cassandra.metrics.Metrics; import org.apache.cassandra.metrics.MetricsRegistry; import com.scylladb.jmx.api.APIClient; +import com.sun.jmx.mbeanserver.JmxMBeanServer; /** * Base type for MBeans containing {@link Metrics}. @@ -47,7 +48,7 @@ public abstract class MetricsMBean extends APIMBean { }; } - private void register(MetricsRegistry registry, MBeanServer server) throws MalformedObjectNameException { + private void register(MetricsRegistry registry, JmxMBeanServer server) throws MalformedObjectNameException { // Check if we're the first/last of our type bound/removed. boolean empty = queryNames(server, getTypePredicate()).isEmpty(); for (Metrics m : metrics) { @@ -63,7 +64,7 @@ public abstract class MetricsMBean extends APIMBean { // Get name etc. name = super.preRegister(server, name); // Register all metrics in server - register(new MetricsRegistry(client, server), server); + register(new MetricsRegistry(client, (JmxMBeanServer) server), (JmxMBeanServer) server); return name; } @@ -77,7 +78,7 @@ public abstract class MetricsMBean extends APIMBean { public void register(Supplier s, ObjectName... objectNames) { for (ObjectName name : objectNames) { try { - server.unregisterMBean(name); + server.getMBeanServerInterceptor().unregisterMBean(name); } catch (MBeanRegistrationException | InstanceNotFoundException e) { } } diff --git a/src/main/java/com/scylladb/jmx/utils/APIBuilder.java b/src/main/java/com/scylladb/jmx/utils/APIBuilder.java index b7fb775..cc16ac4 100644 --- a/src/main/java/com/scylladb/jmx/utils/APIBuilder.java +++ b/src/main/java/com/scylladb/jmx/utils/APIBuilder.java @@ -26,10 +26,14 @@ import javax.management.MBeanServer; import javax.management.MBeanServerBuilder; import javax.management.MBeanServerDelegate; +import com.sun.jmx.mbeanserver.JmxMBeanServer; + public class APIBuilder extends MBeanServerBuilder { @Override public MBeanServer newMBeanServer(String defaultDomain, MBeanServer outer, MBeanServerDelegate delegate) { - MBeanServer nested = super.newMBeanServer(defaultDomain, outer, delegate); + // It is important to set |interceptors| to true while creating the JmxMBeanSearver. + // It is required for calls to JmxMBeanServer.getMBeanServerInterceptor() to be allowed. + JmxMBeanServer nested = (JmxMBeanServer) JmxMBeanServer.newMBeanServer(defaultDomain, outer, delegate, true); return new APIMBeanServer(client, nested); } } \ No newline at end of file diff --git a/src/main/java/com/scylladb/jmx/utils/APIMBeanServer.java b/src/main/java/com/scylladb/jmx/utils/APIMBeanServer.java index 6f2f8f4..9d5ae86 100644 --- a/src/main/java/com/scylladb/jmx/utils/APIMBeanServer.java +++ b/src/main/java/com/scylladb/jmx/utils/APIMBeanServer.java @@ -33,15 +33,16 @@ import org.apache.cassandra.db.ColumnFamilyStore; import org.apache.cassandra.metrics.StreamingMetrics; import com.scylladb.jmx.api.APIClient; +import com.sun.jmx.mbeanserver.JmxMBeanServer; public class APIMBeanServer implements MBeanServer { @SuppressWarnings("unused") private static final Logger logger = Logger.getLogger(APIMBeanServer.class.getName()); private final APIClient client; - private final MBeanServer server; + private final JmxMBeanServer server; - public APIMBeanServer(APIClient client, MBeanServer server) { + public APIMBeanServer(APIClient client, JmxMBeanServer server) { this.client = client; this.server = server; } diff --git a/src/main/java/org/apache/cassandra/db/ColumnFamilyStore.java b/src/main/java/org/apache/cassandra/db/ColumnFamilyStore.java index ea62b14..8cc7bea 100644 --- a/src/main/java/org/apache/cassandra/db/ColumnFamilyStore.java +++ b/src/main/java/org/apache/cassandra/db/ColumnFamilyStore.java @@ -54,6 +54,7 @@ import org.apache.cassandra.metrics.TableMetrics; import com.scylladb.jmx.api.APIClient; import com.scylladb.jmx.metrics.MetricsMBean; +import com.sun.jmx.mbeanserver.JmxMBeanServer; public class ColumnFamilyStore extends MetricsMBean implements ColumnFamilyStoreMBean { private static final Logger logger = Logger.getLogger(ColumnFamilyStore.class.getName()); @@ -102,7 +103,7 @@ public class ColumnFamilyStore extends MetricsMBean implements ColumnFamilyStore "org.apache.cassandra.db:type=" + type + ",keyspace=" + keyspace + ",columnfamily=" + name); } - public static boolean checkRegistration(APIClient client, MBeanServer server) throws MalformedObjectNameException { + public static boolean checkRegistration(APIClient client, JmxMBeanServer server) throws MalformedObjectNameException { JsonArray mbeans = client.getJsonArray("/column_family/"); Set all = new HashSet(); for (int i = 0; i < mbeans.size(); i++) { diff --git a/src/main/java/org/apache/cassandra/metrics/MetricsRegistry.java b/src/main/java/org/apache/cassandra/metrics/MetricsRegistry.java index eee2c30..a17cf80 100644 --- a/src/main/java/org/apache/cassandra/metrics/MetricsRegistry.java +++ b/src/main/java/org/apache/cassandra/metrics/MetricsRegistry.java @@ -39,6 +39,7 @@ import javax.management.NotCompliantMBeanException; import javax.management.ObjectName; import com.scylladb.jmx.api.APIClient; +import com.sun.jmx.mbeanserver.JmxMBeanServer; /** * Makes integrating 3.0 metrics API with 2.0. @@ -53,9 +54,9 @@ public class MetricsRegistry { private static final Logger logger = Logger.getLogger(MetricsRegistry.class.getName()); private final APIClient client; - private final MBeanServer mBeanServer; + private final JmxMBeanServer mBeanServer; - public MetricsRegistry(APIClient client, MBeanServer mBeanServer) { + public MetricsRegistry(APIClient client, JmxMBeanServer mBeanServer) { this.client = client; this.mBeanServer = mBeanServer; } @@ -108,7 +109,7 @@ public class MetricsRegistry { MetricMBean bean = f.get(); for (ObjectName name : objectNames) { try { - mBeanServer.registerMBean(bean, name); + mBeanServer.getMBeanServerInterceptor().registerMBean(bean, name); } catch (InstanceAlreadyExistsException | MBeanRegistrationException | NotCompliantMBeanException e) { logger.log(SEVERE, "Could not register mbean", e); } diff --git a/src/main/java/org/apache/cassandra/metrics/StreamingMetrics.java b/src/main/java/org/apache/cassandra/metrics/StreamingMetrics.java index b2eb35f..fc4fd17 100644 --- a/src/main/java/org/apache/cassandra/metrics/StreamingMetrics.java +++ b/src/main/java/org/apache/cassandra/metrics/StreamingMetrics.java @@ -33,12 +33,12 @@ import java.util.HashSet; import java.util.Set; import javax.json.JsonArray; -import javax.management.MBeanServer; import javax.management.MalformedObjectNameException; import javax.management.ObjectName; import com.scylladb.jmx.api.APIClient; import com.scylladb.jmx.metrics.APIMBean; +import com.sun.jmx.mbeanserver.JmxMBeanServer; /** * Metrics for streaming. @@ -65,11 +65,11 @@ public class StreamingMetrics { return TYPE_NAME.equals(n.getKeyProperty("type")); } - public static void unregister(APIClient client, MBeanServer server) throws MalformedObjectNameException { + public static void unregister(APIClient client, JmxMBeanServer server) throws MalformedObjectNameException { APIMBean.checkRegistration(server, emptySet(), StreamingMetrics::isStreamingName, (n) -> null); } - public static boolean checkRegistration(APIClient client, MBeanServer server) + public static boolean checkRegistration(APIClient client, JmxMBeanServer server) throws MalformedObjectNameException, UnknownHostException { Set all = new HashSet(globalNames); From 48408dc6a3125d5fbff72c63d91cb0eef212168f Mon Sep 17 00:00:00 2001 From: Piotr Jastrzebski Date: Sat, 12 May 2018 18:54:38 +0200 Subject: [PATCH 2/3] Ensure regular ObjectName is returned to remote callers Next patch will introduce new ObjectName implementation that will use less memory. This new object won't be serializable. This means it won't be possible to transport it to a remote caller. We want to keep this new object local to JMX server as well. This patch makes sure that every ObjectName returned from APIBeanServer is transformed into a regular ObjectName. It also makes sure that every ObjectInstance returned from APIBeanServer has its ObjectName swapped with a regular ObjectName. Signed-off-by: Piotr Jastrzebski --- .../scylladb/jmx/utils/APIMBeanServer.java | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/scylladb/jmx/utils/APIMBeanServer.java b/src/main/java/com/scylladb/jmx/utils/APIMBeanServer.java index 9d5ae86..ff84101 100644 --- a/src/main/java/com/scylladb/jmx/utils/APIMBeanServer.java +++ b/src/main/java/com/scylladb/jmx/utils/APIMBeanServer.java @@ -5,6 +5,7 @@ import java.net.UnknownHostException; import java.util.Set; import java.util.logging.Logger; import java.util.regex.Pattern; +import java.util.stream.Collectors; import javax.management.Attribute; import javax.management.AttributeList; @@ -47,37 +48,45 @@ public class APIMBeanServer implements MBeanServer { this.server = server; } + private static ObjectInstance prepareForRemote(final ObjectInstance i) { + return new ObjectInstance(prepareForRemote(i.getObjectName()), i.getClassName()); + } + + private static ObjectName prepareForRemote(final ObjectName n) { + return ObjectName.getInstance(n); + } + @Override public ObjectInstance createMBean(String className, ObjectName name) throws ReflectionException, InstanceAlreadyExistsException, MBeanRegistrationException, MBeanException, NotCompliantMBeanException { - return server.createMBean(className, name); + return prepareForRemote(server.createMBean(className, name)); } @Override public ObjectInstance createMBean(String className, ObjectName name, ObjectName loaderName) throws ReflectionException, InstanceAlreadyExistsException, MBeanRegistrationException, MBeanException, NotCompliantMBeanException, InstanceNotFoundException { - return server.createMBean(className, name, loaderName); + return prepareForRemote(server.createMBean(className, name, loaderName)); } @Override public ObjectInstance createMBean(String className, ObjectName name, Object[] params, String[] signature) throws ReflectionException, InstanceAlreadyExistsException, MBeanRegistrationException, MBeanException, NotCompliantMBeanException { - return server.createMBean(className, name, params, signature); + return prepareForRemote(server.createMBean(className, name, params, signature)); } @Override public ObjectInstance createMBean(String className, ObjectName name, ObjectName loaderName, Object[] params, String[] signature) throws ReflectionException, InstanceAlreadyExistsException, MBeanRegistrationException, MBeanException, NotCompliantMBeanException, InstanceNotFoundException { - return server.createMBean(className, name, loaderName, params, signature); + return prepareForRemote(server.createMBean(className, name, loaderName, params, signature)); } @Override public ObjectInstance registerMBean(Object object, ObjectName name) throws InstanceAlreadyExistsException, MBeanRegistrationException, NotCompliantMBeanException { - return server.registerMBean(object, name); + return prepareForRemote(server.registerMBean(object, name)); } @Override @@ -88,19 +97,19 @@ public class APIMBeanServer implements MBeanServer { @Override public ObjectInstance getObjectInstance(ObjectName name) throws InstanceNotFoundException { checkRegistrations(name); - return server.getObjectInstance(name); + return prepareForRemote(server.getObjectInstance(name)); } @Override public Set queryNames(ObjectName name, QueryExp query) { checkRegistrations(name); - return server.queryNames(name, query); + return server.queryNames(name, query).stream().map(n -> prepareForRemote(n)).collect(Collectors.toSet()); } @Override public Set queryMBeans(ObjectName name, QueryExp query) { checkRegistrations(name); - return server.queryMBeans(name, query); + return server.queryMBeans(name, query).stream().map(i -> prepareForRemote(i)).collect(Collectors.toSet()); } @Override From 455f5717eaa40fd63451f6bf828857e96fa628b1 Mon Sep 17 00:00:00 2001 From: Piotr Jastrzebski Date: Sat, 12 May 2018 19:08:37 +0200 Subject: [PATCH 3/3] Introduce and use TableMetricObjectName This is a new extention of ObjectName that uses less memory. TableMetricNameFactory and AllTableMetricNameFactory can create it instead of regular ObjectName to save memory. It is possible to save memory because each name created by TableMetricNameFactory (or AllTableMetricNameFactory) shares most of its data with other names created by the same factory and there's no need to create multiple copies. Signed-off-by: Piotr Jastrzebski --- .../cassandra/metrics/TableMetrics.java | 193 ++++++++++++++++-- 1 file changed, 178 insertions(+), 15 deletions(-) diff --git a/src/main/java/org/apache/cassandra/metrics/TableMetrics.java b/src/main/java/org/apache/cassandra/metrics/TableMetrics.java index ee48e71..1faf465 100644 --- a/src/main/java/org/apache/cassandra/metrics/TableMetrics.java +++ b/src/main/java/org/apache/cassandra/metrics/TableMetrics.java @@ -19,6 +19,7 @@ package org.apache.cassandra.metrics; import static com.scylladb.jmx.api.APIClient.getReader; +import java.util.Hashtable; import java.util.function.BiFunction; import java.util.function.Function; import java.util.function.Supplier; @@ -27,7 +28,6 @@ import javax.management.MalformedObjectNameException; import javax.management.ObjectName; import org.apache.cassandra.db.ColumnFamilyStore; -import org.apache.cassandra.metrics.MetricsRegistry.MetricMBean; import com.scylladb.jmx.api.APIClient; @@ -266,7 +266,93 @@ public class TableMetrics implements Metrics { registry.createTableCounter("RowCacheMiss", "row_cache_miss"); } - static class TableMetricNameFactory implements MetricNameFactory { + static class TableMetricObjectName extends javax.management.ObjectName { + private static final String FAKE_NAME = "a:a=a"; + + private final TableMetricStringNameFactory factory; + private final String metricName; + + public TableMetricObjectName(TableMetricStringNameFactory factory, String metricName) throws MalformedObjectNameException { + super(FAKE_NAME); + this.factory = factory; + this.metricName = metricName; + } + + @Override + public boolean isPropertyValuePattern(String property) { + return false; + } + + @Override + public String getCanonicalName() { + return factory.createMetricStringName(metricName); + } + + @Override + public String getDomain() { + return factory.getDomain(); + } + + @Override + public String getKeyProperty(String property) { + if (property == "name") { + return metricName; + } + return factory.getKeyProperty(property); + } + + @Override + public Hashtable getKeyPropertyList() { + Hashtable res = factory.getKeyPropertyList(); + res.put("name", metricName); + return res; + } + + @Override + public String getKeyPropertyListString() { + return factory.getKeyPropertyListString(metricName); + } + + @Override + public String getCanonicalKeyPropertyListString() { + return getKeyPropertyListString(); + } + + @Override + public String toString() { + return getCanonicalName(); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof TableMetricObjectName)) return false; + return getCanonicalName().equals(((TableMetricObjectName) o).getCanonicalName()); + } + + @Override + public int hashCode() { + return getCanonicalName().hashCode(); + } + + @Override + public boolean apply(ObjectName name) { + if (name.isDomainPattern() || name.isPropertyListPattern() || name.isPropertyValuePattern()) { + return false; + } + return getCanonicalName().equals(name.getCanonicalName()); + } + } + + static interface TableMetricStringNameFactory { + String createMetricStringName(String metricName); + String getDomain(); + String getKeyProperty(String property); + Hashtable getKeyPropertyList(); + String getKeyPropertyListString(String metricName); + } + + static class TableMetricNameFactory implements MetricNameFactory, TableMetricStringNameFactory { private final String keyspaceName; private final String tableName; private final boolean isIndex; @@ -279,37 +365,114 @@ public class TableMetrics implements Metrics { this.type = type; } - @Override - public ObjectName createMetricName(String metricName) throws MalformedObjectNameException { - String groupName = TableMetrics.class.getPackage().getName(); + private void appendKeyPropertyListString(final StringBuilder sb, final String metricName) { String type = isIndex ? "Index" + this.type : this.type; + // Order matters here - keys have to be sorted + sb.append("keyspace=").append(keyspaceName); + sb.append(",name=").append(metricName); + sb.append(",scope=").append(tableName); + sb.append(",type=").append(type); + } + + @Override + public String createMetricStringName(String metricName) { + String groupName = TableMetrics.class.getPackage().getName(); StringBuilder mbeanName = new StringBuilder(); mbeanName.append(groupName).append(":"); - mbeanName.append("type=").append(type); - mbeanName.append(",keyspace=").append(keyspaceName); - mbeanName.append(",scope=").append(tableName); - mbeanName.append(",name=").append(metricName); + appendKeyPropertyListString(mbeanName, metricName); + return mbeanName.toString(); + } - return new ObjectName(mbeanName.toString()); + @Override + public String getDomain() { + return TableMetrics.class.getPackage().getName(); + } + + @Override + public String getKeyProperty(String property) { + switch (property) { + case "keyspace": return keyspaceName; + case "scope": return tableName; + case "type": return type; + default: return null; + } + } + + @Override + public Hashtable getKeyPropertyList() { + Hashtable res = new Hashtable<>(); + res.put("keyspace", keyspaceName); + res.put("scope", tableName); + res.put("type", type); + return res; + } + + @Override + public String getKeyPropertyListString(String metricName) { + final StringBuilder sb = new StringBuilder(); + appendKeyPropertyListString(sb, metricName); + return sb.toString(); + } + + @Override + public ObjectName createMetricName(String metricName) throws MalformedObjectNameException { + return new TableMetricObjectName(this, metricName); } } - static class AllTableMetricNameFactory implements MetricNameFactory { + static class AllTableMetricNameFactory implements MetricNameFactory, TableMetricStringNameFactory { private final String type; public AllTableMetricNameFactory(String type) { this.type = type; } + private void appendKeyPropertyListString(final StringBuilder sb, final String metricName) { + // Order matters here - keys have to be sorted + sb.append("name=").append(metricName); + sb.append(",type=" + type); + } + @Override - public ObjectName createMetricName(String metricName) throws MalformedObjectNameException { + public String createMetricStringName(String metricName) { String groupName = TableMetrics.class.getPackage().getName(); StringBuilder mbeanName = new StringBuilder(); mbeanName.append(groupName).append(":"); - mbeanName.append("type=" + type); - mbeanName.append(",name=").append(metricName); - return new ObjectName(mbeanName.toString()); + appendKeyPropertyListString(mbeanName, metricName); + return mbeanName.toString(); + } + + @Override + public String getDomain() { + return TableMetrics.class.getPackage().getName(); + } + + @Override + public String getKeyProperty(String property) { + switch (property) { + case "type": return type; + default: return null; + } + } + + @Override + public Hashtable getKeyPropertyList() { + Hashtable res = new Hashtable<>(); + res.put("type", type); + return res; + } + + @Override + public String getKeyPropertyListString(String metricName) { + final StringBuilder sb = new StringBuilder(); + appendKeyPropertyListString(sb, metricName); + return sb.toString(); + } + + @Override + public ObjectName createMetricName(String metricName) throws MalformedObjectNameException { + return new TableMetricObjectName(this, metricName); } }