frankgh commented on code in PR #2318:
URL: https://github.com/apache/cassandra/pull/2318#discussion_r1190132193
##########
build.xml:
##########
@@ -263,6 +265,8 @@
<string>--add-opens java.base/java.math=ALL-UNNAMED</string>
<string>--add-opens java.base/java.lang.reflect=ALL-UNNAMED</string>
<string>--add-opens java.base/java.net=ALL-UNNAMED</string>
+
+ <string>--add-opens java.rmi/sun.rmi.transport.tcp=ALL-UNNAMED</string>
Review Comment:
is this `--add-opens` scoped to the in-jvm dtests only?
##########
src/java/org/apache/cassandra/config/CassandraRelevantProperties.java:
##########
@@ -219,6 +219,8 @@
/** This property indicates whether disable_mbean_registration is true */
IS_DISABLED_MBEAN_REGISTRATION("org.apache.cassandra.disable_mbean_registration"),
+ /** This property indicates if the code is running under the in-jvm dtest
framework */
Review Comment:
does the order of the enum matter here? Some enums might be sensitive to the
order, so usually new enums are added at the end. It might not be the case here
##########
src/java/org/apache/cassandra/utils/MBeanWrapper.java:
##########
@@ -170,6 +186,125 @@ public void unregisterMBean(ObjectName mbeanName,
OnException onException)
}
}
+ static class InstanceMBeanWrapper implements MBeanWrapper
+ {
+ private final MBeanServer mbs;
+ public final UUID id = UUID.randomUUID();
+
+ public InstanceMBeanWrapper(String hostname)
+ {
+ mbs = MBeanServerFactory.createMBeanServer(hostname + "-" + id);
+ }
+
+ public void registerMBean(Object obj, ObjectName mbeanName,
OnException onException)
+ {
+ try
+ {
+ mbs.registerMBean(obj, mbeanName);
+ }
+ catch (Exception e)
+ {
+ onException.handler.accept(e);
+ }
+ }
+
+ public boolean isRegistered(ObjectName mbeanName, OnException
onException)
+ {
+ try
+ {
+ return mbs.isRegistered(mbeanName);
+ }
+ catch (Exception e)
+ {
+ onException.handler.accept(e);
+ }
+ return false;
+ }
+
+ public void unregisterMBean(ObjectName mbeanName, OnException
onException)
+ {
+ try
+ {
+ mbs.unregisterMBean(mbeanName);
+ }
+ catch (Exception e)
+ {
+ onException.handler.accept(e);
+ }
+ }
+
+ public MBeanServer getMBeanServer()
+ {
+ return mbs;
+ }
+
+ public void close() {
+ mbs.queryNames(null, null).forEach(name -> {
+ try {
+ if
(!name.getCanonicalName().contains("MBeanServerDelegate"))
+ mbs.unregisterMBean(name);
+ } catch (Throwable e) {
+ logger.debug("Could not unregister mbean {}",
name.getCanonicalName());
+ }
+ });
+ MBeanServerFactory.releaseMBeanServer(mbs);
+ }
+ }
+
+ static class DelegatingMbeanWrapper implements MBeanWrapper {
+ MBeanWrapper delegate;
+
+ public DelegatingMbeanWrapper(MBeanWrapper mBeanWrapper)
+ {
+ delegate = mBeanWrapper;
+ }
+
+ public void setDelegate(MBeanWrapper wrapper) {
+ delegate = wrapper;
+ }
+
+ public MBeanWrapper getDelegate()
+ {
+ return delegate;
+ }
Review Comment:
new line after brace?
```suggestion
}
```
##########
test/distributed/org/apache/cassandra/distributed/impl/Instance.java:
##########
@@ -754,6 +786,146 @@ else if (cluster instanceof Cluster)
initialized = true;
}
+ private void startJmx()
+ {
+ try
+ {
+ IS_DISABLED_MBEAN_REGISTRATION.setBoolean(false);
+ InetAddress addr = config.broadcastAddress().getAddress();
+
+ int jmxPort = config.jmxPort();
+
+ String hostname = addr.getHostAddress();
+ wrapper = new MBeanWrapper.InstanceMBeanWrapper(hostname + ":" +
jmxPort);
+ ((MBeanWrapper.DelegatingMbeanWrapper)
MBeanWrapper.instance).setDelegate(wrapper);
+ Map<String, Object> env = new HashMap<>();
+
+ serverSocketFactory = new
CollectingRMIServerSocketFactoryImpl(addr);
+ env.put(RMIConnectorServer.RMI_SERVER_SOCKET_FACTORY_ATTRIBUTE,
+ serverSocketFactory);
+ clientSocketFactory = new RMIClientSocketFactoryImpl(addr);
+ env.put(RMIConnectorServer.RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE,
+ clientSocketFactory);
+
+ // configure the RMI registry
+ registry = new JMXServerUtils.JmxRegistry(jmxPort,
+ clientSocketFactory,
+ serverSocketFactory,
+ "jmxrmi");
+
+ // Mark the JMX server as a permanently exported object. This
allows the JVM to exit with the
+ // server running and also exempts it from the distributed GC
scheduler which otherwise would
+ // potentially attempt a full GC every
`sun.rmi.dgc.server.gcInterval` millis (default is 3600000ms)
+ // For more background see:
+ // - CASSANDRA-2967
+ // - https://www.jclarity.com/2015/01/27/rmi-system-gc-unplugged/
+ // - https://bugs.openjdk.java.net/browse/JDK-6760712
+ env.put("jmx.remote.x.daemon", "true");
+
+ // Set the port used to create subsequent connections to exported
objects over RMI. This simplifies
+ // configuration in firewalled environments, but it can't be used
in conjuction with SSL sockets.
+ // See: CASSANDRA-7087
+ int rmiPort = COM_SUN_MANAGEMENT_JMXREMOTE_RMI_PORT.getInt();
+
+ // We create the underlying RMIJRMPServerImpl so that we can
manually bind it to the registry,
+ // rather then specifying a binding address in the JMXServiceURL
and letting it be done automatically
+ // when the server is started. The reason for this is that if the
registry is configured with SSL
+ // sockets, the JMXConnectorServer acts as its client during the
binding which means it needs to
+ // have a truststore configured which contains the registry's
certificate. Manually binding removes
+ // this problem.
+ // See CASSANDRA-12109.
+ jmxRmiServer = new RMIJRMPServerImpl(rmiPort,
+ (RMIClientSocketFactory)
env.get(RMIConnectorServer.RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE),
+ (RMIServerSocketFactory)
env.get(RMIConnectorServer.RMI_SERVER_SOCKET_FACTORY_ATTRIBUTE),
+ env);
+ JMXServiceURL serviceURL = new JMXServiceURL("rmi", hostname,
rmiPort);
+ jmxConnectorServer = new RMIConnectorServer(serviceURL, env,
jmxRmiServer, wrapper.getMBeanServer());
+
+ jmxConnectorServer.start();
+
+ IS_DISABLED_MBEAN_REGISTRATION.setBoolean(false);
+
+ registry.setRemoteServerStub(jmxRmiServer.toStub());
+ JMXServerUtils.logJmxServiceUrl(addr, jmxPort);
+ }
+ catch (IOException e)
+ {
+ throw new RuntimeException("Feature.JMX was enabled but could not
be started.", e);
+ }
+ }
+
+ private void stopJmx() throws ClassNotFoundException,
IllegalAccessException, InvocationTargetException, NoSuchMethodException,
IOException, NoSuchFieldException, InterruptedException
+ {
+ if (!config.has(JMX))
+ return;
+ // First, swap the mbean wrapper back to a NoOp wrapper
+ // This prevents later attempts to unregister mbeans from failing in
Cassandra code, as we're going to
+ // unregister all of them here
+
((MBeanWrapper.DelegatingMbeanWrapper)MBeanWrapper.instance).setDelegate(new
MBeanWrapper.NoOpMBeanWrapper());
+ try {
Review Comment:
seems like formatting is off for this method, for example
```suggestion
try
{
```
##########
src/java/org/apache/cassandra/utils/RMIClientSocketFactoryImpl.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.utils;
+
+import java.io.IOException;
+import java.io.Serializable;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.rmi.server.RMIClientSocketFactory;
+
+/**
+ * This class is used to override the local address the JMX client calculates
when trying to connect,
+ * which can otherwise be influences by the system property
"java.rmi.server.hostname" in strange and
Review Comment:
```suggestion
* which can otherwise be influenced by the system property
"java.rmi.server.hostname" in strange and
```
##########
src/java/org/apache/cassandra/utils/JMXServerUtils.java:
##########
@@ -362,5 +365,24 @@ public String[] list() throws RemoteException {
public void setRemoteServerStub(Remote remoteServerStub) {
this.remoteServerStub = remoteServerStub;
}
+
+ /**
+ * Closes the underlying JMX registry by unexporting this instance.
+ * There is no reason to do this except for in-jvm dtests where we need
+ * to stop the registry, so we can start with a clean slate for future
cluster
+ * builds, and the superclass never expects to be shut down and
therefore doesn't
+ * handle this edge case at all.
+ */
+ @VisibleForTesting
+ public void close() {
+ try
+ {
+ UnicastRemoteObject.unexportObject(this, true);
+ }
+ catch (NoSuchObjectException e)
Review Comment:
maybe rename the variable to indicate it's ignored
```suggestion
catch (NoSuchObjectException ignored)
```
##########
src/java/org/apache/cassandra/utils/MBeanWrapper.java:
##########
@@ -170,6 +186,125 @@ public void unregisterMBean(ObjectName mbeanName,
OnException onException)
}
}
+ static class InstanceMBeanWrapper implements MBeanWrapper
+ {
+ private final MBeanServer mbs;
+ public final UUID id = UUID.randomUUID();
+
+ public InstanceMBeanWrapper(String hostname)
+ {
+ mbs = MBeanServerFactory.createMBeanServer(hostname + "-" + id);
+ }
+
+ public void registerMBean(Object obj, ObjectName mbeanName,
OnException onException)
Review Comment:
can we add the `@Override` annotation to distinguish between internal
methods and overridden methods?
##########
test/distributed/org/apache/cassandra/distributed/impl/INodeProvisionStrategy.java:
##########
@@ -56,6 +56,11 @@ public int nativeTransportPort(int nodeNum)
{
return 9041 + nodeNum;
}
+
+ public int jmxPort(int nodeNum)
Review Comment:
`@Override` annotation?
##########
src/java/org/apache/cassandra/utils/MBeanWrapper.java:
##########
@@ -170,6 +186,125 @@ public void unregisterMBean(ObjectName mbeanName,
OnException onException)
}
}
+ static class InstanceMBeanWrapper implements MBeanWrapper
+ {
+ private final MBeanServer mbs;
+ public final UUID id = UUID.randomUUID();
+
+ public InstanceMBeanWrapper(String hostname)
+ {
+ mbs = MBeanServerFactory.createMBeanServer(hostname + "-" + id);
+ }
+
+ public void registerMBean(Object obj, ObjectName mbeanName,
OnException onException)
+ {
+ try
+ {
+ mbs.registerMBean(obj, mbeanName);
+ }
+ catch (Exception e)
+ {
+ onException.handler.accept(e);
+ }
+ }
+
+ public boolean isRegistered(ObjectName mbeanName, OnException
onException)
+ {
+ try
+ {
+ return mbs.isRegistered(mbeanName);
+ }
+ catch (Exception e)
+ {
+ onException.handler.accept(e);
+ }
+ return false;
+ }
+
+ public void unregisterMBean(ObjectName mbeanName, OnException
onException)
+ {
+ try
+ {
+ mbs.unregisterMBean(mbeanName);
+ }
+ catch (Exception e)
+ {
+ onException.handler.accept(e);
+ }
+ }
+
+ public MBeanServer getMBeanServer()
+ {
+ return mbs;
+ }
+
+ public void close() {
+ mbs.queryNames(null, null).forEach(name -> {
+ try {
+ if
(!name.getCanonicalName().contains("MBeanServerDelegate"))
+ mbs.unregisterMBean(name);
+ } catch (Throwable e) {
+ logger.debug("Could not unregister mbean {}",
name.getCanonicalName());
Review Comment:
log at warn maybe?
```suggestion
logger.warn("Could not unregister mbean {}",
name.getCanonicalName(), e);
```
##########
test/distributed/org/apache/cassandra/distributed/impl/Instance.java:
##########
@@ -754,6 +786,146 @@ else if (cluster instanceof Cluster)
initialized = true;
}
+ private void startJmx()
+ {
+ try
+ {
+ IS_DISABLED_MBEAN_REGISTRATION.setBoolean(false);
+ InetAddress addr = config.broadcastAddress().getAddress();
+
+ int jmxPort = config.jmxPort();
+
+ String hostname = addr.getHostAddress();
+ wrapper = new MBeanWrapper.InstanceMBeanWrapper(hostname + ":" +
jmxPort);
+ ((MBeanWrapper.DelegatingMbeanWrapper)
MBeanWrapper.instance).setDelegate(wrapper);
+ Map<String, Object> env = new HashMap<>();
+
+ serverSocketFactory = new
CollectingRMIServerSocketFactoryImpl(addr);
+ env.put(RMIConnectorServer.RMI_SERVER_SOCKET_FACTORY_ATTRIBUTE,
+ serverSocketFactory);
+ clientSocketFactory = new RMIClientSocketFactoryImpl(addr);
+ env.put(RMIConnectorServer.RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE,
+ clientSocketFactory);
+
+ // configure the RMI registry
+ registry = new JMXServerUtils.JmxRegistry(jmxPort,
+ clientSocketFactory,
+ serverSocketFactory,
+ "jmxrmi");
+
+ // Mark the JMX server as a permanently exported object. This
allows the JVM to exit with the
+ // server running and also exempts it from the distributed GC
scheduler which otherwise would
+ // potentially attempt a full GC every
`sun.rmi.dgc.server.gcInterval` millis (default is 3600000ms)
+ // For more background see:
+ // - CASSANDRA-2967
+ // - https://www.jclarity.com/2015/01/27/rmi-system-gc-unplugged/
+ // - https://bugs.openjdk.java.net/browse/JDK-6760712
+ env.put("jmx.remote.x.daemon", "true");
+
+ // Set the port used to create subsequent connections to exported
objects over RMI. This simplifies
+ // configuration in firewalled environments, but it can't be used
in conjuction with SSL sockets.
+ // See: CASSANDRA-7087
+ int rmiPort = COM_SUN_MANAGEMENT_JMXREMOTE_RMI_PORT.getInt();
+
+ // We create the underlying RMIJRMPServerImpl so that we can
manually bind it to the registry,
+ // rather then specifying a binding address in the JMXServiceURL
and letting it be done automatically
+ // when the server is started. The reason for this is that if the
registry is configured with SSL
+ // sockets, the JMXConnectorServer acts as its client during the
binding which means it needs to
+ // have a truststore configured which contains the registry's
certificate. Manually binding removes
+ // this problem.
+ // See CASSANDRA-12109.
+ jmxRmiServer = new RMIJRMPServerImpl(rmiPort,
+ (RMIClientSocketFactory)
env.get(RMIConnectorServer.RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE),
+ (RMIServerSocketFactory)
env.get(RMIConnectorServer.RMI_SERVER_SOCKET_FACTORY_ATTRIBUTE),
+ env);
+ JMXServiceURL serviceURL = new JMXServiceURL("rmi", hostname,
rmiPort);
+ jmxConnectorServer = new RMIConnectorServer(serviceURL, env,
jmxRmiServer, wrapper.getMBeanServer());
+
+ jmxConnectorServer.start();
+
+ IS_DISABLED_MBEAN_REGISTRATION.setBoolean(false);
+
+ registry.setRemoteServerStub(jmxRmiServer.toStub());
+ JMXServerUtils.logJmxServiceUrl(addr, jmxPort);
+ }
+ catch (IOException e)
+ {
+ throw new RuntimeException("Feature.JMX was enabled but could not
be started.", e);
+ }
+ }
+
+ private void stopJmx() throws ClassNotFoundException,
IllegalAccessException, InvocationTargetException, NoSuchMethodException,
IOException, NoSuchFieldException, InterruptedException
+ {
+ if (!config.has(JMX))
+ return;
+ // First, swap the mbean wrapper back to a NoOp wrapper
+ // This prevents later attempts to unregister mbeans from failing in
Cassandra code, as we're going to
+ // unregister all of them here
+
((MBeanWrapper.DelegatingMbeanWrapper)MBeanWrapper.instance).setDelegate(new
MBeanWrapper.NoOpMBeanWrapper());
+ try {
+ wrapper.close();
+ } catch (Throwable e) {
+ inInstancelogger.warn("failed to close wrapper.", e);
+ }
+ try {
+ jmxConnectorServer.stop();
+ } catch (Throwable e) {
+ inInstancelogger.warn("failed to close jmxConnectorServer.", e);
+ }
+ try {
+ registry.close();
+ } catch (Throwable e) {
+ inInstancelogger.warn("failed to close registry.", e);
+ }
+ try {
+ serverSocketFactory.close();
+ } catch (Throwable e) {
+ inInstancelogger.warn("failed to close serverSocketFactory.", e);
+ }
+ // The TCPEndpoint class holds references to a class in the in-jvm
dtest framework
+ // which transitively has a reference to the InstanceClassLoader, so
we need to
+ // make sure to remove the reference to them when the instance is
shutting down
+ clearMapField(TCPEndpoint.class, null, "localEndpoints",
this::endpointCreatedByThisClassloader);
+ Thread.sleep(2000); // Allow JMX background processes time to stop
+ }
+
+ /**
+ * Checks to make sure the endpoint in question was created by the
instance classloader
+ * for this instance to prevent us from over-removing items.
+ * @param entry The map entry to be checked
+ * @return true, if the TCPEndpoint in the map entry was created by the
InstanceClassloader instance
+ * created by this Instance. Otherwise, false.
+ */
+ private boolean endpointCreatedByThisClassloader(Map.Entry<TCPEndpoint,
LinkedList> entry) {
Review Comment:
formatting?
```suggestion
private boolean endpointCreatedByThisClassloader(Map.Entry<TCPEndpoint,
LinkedList> entry)
{
```
##########
build.xml:
##########
@@ -207,6 +207,7 @@
<string>--add-exports
java.management.rmi/com.sun.jmx.remote.internal.rmi=ALL-UNNAMED</string>
<string>--add-exports java.rmi/sun.rmi.registry=ALL-UNNAMED</string>
<string>--add-exports java.rmi/sun.rmi.server=ALL-UNNAMED</string>
+ <string>--add-exports
java.rmi/sun.rmi.transport.tcp=ALL-UNNAMED</string>
Review Comment:
is this `--add-opens` scoped to the in-jvm dtests only?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
