gemmellr commented on code in PR #4401:
URL: https://github.com/apache/activemq-artemis/pull/4401#discussion_r1137156047
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/BridgeControlTest.java:
##########
@@ -139,7 +138,7 @@ public void setUp() throws Exception {
Configuration conf_0 = createBasicConfig().addAcceptorConfiguration(new
TransportConfiguration(InVMAcceptorFactory.class.getName())).addConnectorConfiguration(connectorConfig.getName(),
connectorConfig).addQueueConfiguration(sourceQueueConfig).addBridgeConfiguration(bridgeConfig);
- server_1 = addServer(ActiveMQServers.newActiveMQServer(conf_1,
MBeanServerFactory.createMBeanServer(), false));
+ server_1 = addServer(ActiveMQServers.newActiveMQServer(conf_1,
getMBeanServer(), false));
Review Comment:
This would now be using the same MBeanServer as the other call below using
the variable, mbeanServer, coming from the parent, rather than using two
differnet MBeanServers as the different variables suggests...so you might as
well make the two consistently use 1 if its acceptable for them to actually be
using the same MBeanServer.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/BridgeControlUsingCoreTest.java:
##########
@@ -116,7 +115,7 @@ public void setUp() throws Exception {
Configuration conf_0 = createBasicConfig().addAcceptorConfiguration(new
TransportConfiguration(INVM_ACCEPTOR_FACTORY)).addConnectorConfiguration(connectorConfig.getName(),
connectorConfig).addQueueConfiguration(sourceQueueConfig).addBridgeConfiguration(bridgeConfig);
- server_1 = addServer(ActiveMQServers.newActiveMQServer(conf_1,
MBeanServerFactory.createMBeanServer(), false));
+ server_1 = addServer(ActiveMQServers.newActiveMQServer(conf_1,
getMBeanServer(), false));
Review Comment:
Ditto
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ClusterConnectionControl2Test.java:
##########
@@ -118,7 +117,7 @@ public void setUp() throws Exception {
Configuration conf_0 =
createBasicConfig(1).addClusterConfiguration(clusterConnectionConfig_0).addAcceptorConfiguration(acceptorConfig_0).addConnectorConfiguration("netty",
connectorConfig_0).addDiscoveryGroupConfiguration(discoveryName,
discoveryGroupConfig).addBroadcastGroupConfiguration(broadcastGroupConfig);
- mbeanServer_1 = MBeanServerFactory.createMBeanServer();
+ mbeanServer_1 = getMBeanServer();
Review Comment:
Basically the same
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ClusterConnectionControlTest.java:
##########
@@ -206,7 +205,7 @@ public void setUp() throws Exception {
Configuration conf_0 = createBasicConfig().addAcceptorConfiguration(new
TransportConfiguration(InVMAcceptorFactory.class.getName())).addConnectorConfiguration(connectorConfig.getName(),
connectorConfig).addClusterConfiguration(clusterConnectionConfig1).addClusterConfiguration(clusterConnectionConfig2).addDiscoveryGroupConfiguration(discoveryGroupName,
discoveryGroupConfig);
- mbeanServer_1 = MBeanServerFactory.createMBeanServer();
+ mbeanServer_1 = getMBeanServer();
Review Comment:
..and again..
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ManagementTestBase.java:
##########
@@ -71,17 +70,12 @@ protected static void consumeMessages(final int expected,
public void setUp() throws Exception {
super.setUp();
- createMBeanServer();
- }
-
- protected void createMBeanServer() {
- mbeanServer = MBeanServerFactory.createMBeanServer();
+ getMBeanServer();
Review Comment:
This seems like a redundant duplicate.
##########
artemis-server/src/test/java/org/apache/activemq/artemis/tests/util/ActiveMQTestBase.java:
##########
@@ -176,6 +178,15 @@
*/
public abstract class ActiveMQTestBase extends Assert {
+ // MBeanServer is an expensive object.
+ // Even if you called unregisterMBeanServer it will still be allocated
somewhere in the VM.
+ // this will make the testsuite a lot lighter
+ static MBeanServer mBeanServer = MBeanServerFactory.createMBeanServer();
Review Comment:
If all the tests are largely going to use the same MBeanServer, for all
their brokers, I'd wonder if they couldn't just use the
probably-already-existing _PlatformMBeanServer_ and remove need for this?
https://docs.oracle.com/javase/8/docs/api/java/lang/management/ManagementFactory.html#getPlatformMBeanServer--
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/util/JMSClusteredTestBase.java:
##########
@@ -119,7 +118,7 @@ private void setupServer2() throws Exception {
JMSConfigurationImpl jmsconfig = new JMSConfigurationImpl();
- mBeanServer2 = MBeanServerFactory.createMBeanServer();
+ mBeanServer2 = getMBeanServer();
Review Comment:
As in other cases, misleading having 2 variables both using the same actual
MBeanServer
--
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]
