You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/08/18 14:49:52 UTC
[GitHub] [geode] jchen21 opened a new pull request #6770: Skip setting MBeanServer when JMXConnectorServer will set it
jchen21 opened a new pull request #6770:
URL: https://github.com/apache/geode/pull/6770
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #6770: GEODE-9515: Skip setting MBeanServer when JMXConnectorServer will set it
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #6770:
URL: https://github.com/apache/geode/pull/6770#discussion_r693154201
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
##########
@@ -449,20 +463,15 @@ public synchronized void start() throws IOException {
if (accessFile != null && accessFile.length() > 0) {
// Rewire the mbs hierarchy to set accessController
ReadOpFileAccessController controller = new ReadOpFileAccessController(accessFile);
- controller.setMBeanServer(mbs);
+ if (jmxConnectorServer.getMBeanServer() == null) {
Review comment:
Agree.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #6770: GEODE-9515: Skip setting MBeanServer when JMXConnectorServer will set it
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #6770:
URL: https://github.com/apache/geode/pull/6770#discussion_r692482329
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
##########
@@ -449,20 +463,15 @@ public synchronized void start() throws IOException {
if (accessFile != null && accessFile.length() > 0) {
// Rewire the mbs hierarchy to set accessController
ReadOpFileAccessController controller = new ReadOpFileAccessController(accessFile);
- controller.setMBeanServer(mbs);
+ if (jmxConnectorServer.getMBeanServer() == null) {
Review comment:
Generally, I don't expect the MBeanServer to be null for `jmxConnectorServer`. The MBeanServer is from line 369 and put in the `jmxConnectorServer` constructor in 416. This `if` statement is a guard in case some other code path leads to a null MBeanServer in `jmxConnectorServer`, then we still need to set the MBeanServer for the `controller`. However, we don't want to set MBeanServer twice for the `controller`, which is the original bug.
##########
File path: geode-core/src/test/java/org/apache/geode/management/internal/ManagementAgentTest.java
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.geode.management.internal;
+
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.File;
+import java.io.IOException;
+import java.lang.management.ManagementFactory;
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.management.MBeanServer;
+import javax.management.remote.JMXConnectorServer;
+import javax.management.remote.JMXServiceURL;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import org.apache.geode.distributed.internal.DistributionConfig;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.security.SecurityService;
+
+public class ManagementAgentTest {
+
+ // private ManagementAgent managementAgent = spy(ManagementAgent.class);
+ @Rule
+ public TemporaryFolder temporaryFolder = new TemporaryFolder();
Review comment:
Agree.
##########
File path: geode-core/src/test/java/org/apache/geode/management/internal/ManagementAgentTest.java
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.geode.management.internal;
+
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.File;
+import java.io.IOException;
+import java.lang.management.ManagementFactory;
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.management.MBeanServer;
+import javax.management.remote.JMXConnectorServer;
+import javax.management.remote.JMXServiceURL;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import org.apache.geode.distributed.internal.DistributionConfig;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.security.SecurityService;
+
+public class ManagementAgentTest {
+
+ // private ManagementAgent managementAgent = spy(ManagementAgent.class);
+ @Rule
+ public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+ @Test
+ public void testSetMBeanServer() throws IOException {
+ DistributionConfig distributionConfig = mock(DistributionConfig.class);
+ InternalCache internalCache = mock(InternalCache.class);
+ JmxRmiSerialFilter serialFilter = mock(JmxRmiSerialFilter.class);
+ SecurityService securityService = mock(SecurityService.class);
+ when(internalCache.getSecurityService()).thenReturn(securityService);
+ when(securityService.isIntegratedSecurity()).thenReturn(false);
+ File tempFile = temporaryFolder.newFile("testFile");
+ when(distributionConfig.getJmxManagerAccessFile()).thenReturn(tempFile.getCanonicalPath());
+ ManagementAgent managementAgent =
+ new ManagementAgent(distributionConfig, internalCache, serialFilter);
+ MBeanServer mBeanServer = mock(MBeanServer.class);
+ JMXConnectorServer jmxConnectorServerWithMBeanServer = new JMXConnectorServer(mBeanServer) {
Review comment:
I need two instances of subclass of `JMXConnectorServer`, which an abstract class. One instance with a non-null `mbeanServer` field, the other with a null `mbeanServer`. I don't know why move this to a static inner-class. I have tried to mock `JMXConnectorServer`. But I don't think a mock will work in this test case. I need to set the `mbeanServer` field of `JMXConnectorServer`. There is no setter for it. Only the constructor sets it.
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
##########
@@ -508,6 +517,10 @@ public JMXConnectorServer getJmxConnectorServer() {
return jmxConnectorServer;
}
+ public void setJmxConnectorServer(JMXConnectorServer jmxConnectorServer) {
+ this.jmxConnectorServer = jmxConnectorServer;
+ }
+
Review comment:
This method is added for the test. So it should be non-private. We can remove the `public` scope.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #6770: GEODE-9515: Skip setting MBeanServer when JMXConnectorServer will set it
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #6770:
URL: https://github.com/apache/geode/pull/6770#discussion_r693101046
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
##########
@@ -449,20 +463,15 @@ public synchronized void start() throws IOException {
if (accessFile != null && accessFile.length() > 0) {
// Rewire the mbs hierarchy to set accessController
ReadOpFileAccessController controller = new ReadOpFileAccessController(accessFile);
- controller.setMBeanServer(mbs);
+ if (jmxConnectorServer.getMBeanServer() == null) {
Review comment:
We know at this point `JMXConnectorServer`'s mbeanServer is definitely not null, so inside the call `setMBeanServerForwarder`, it will call `mbsf.setMBeanServer(mbeanServer)` anyway, should we just delete line 452 then?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #6770: GEODE-9515: Skip setting MBeanServer when JMXConnectorServer will set it
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #6770:
URL: https://github.com/apache/geode/pull/6770#discussion_r692499907
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
##########
@@ -449,20 +463,15 @@ public synchronized void start() throws IOException {
if (accessFile != null && accessFile.length() > 0) {
// Rewire the mbs hierarchy to set accessController
ReadOpFileAccessController controller = new ReadOpFileAccessController(accessFile);
- controller.setMBeanServer(mbs);
+ if (jmxConnectorServer.getMBeanServer() == null) {
Review comment:
If you look at the implementation of `JMXConnectorServer. setMBeanServerForwarder()`, if the MBeanServer of the connector server is non-null, it will set the MBeanServer for the forwarder, which is the `controller` in our case. We want to avoid setting the MBeanServer twice for the controller, otherwise it will throw `IllegalArgumentException`.
Here is the code of `JMXConnectorServer. setMBeanServerForwarder()`:
```
public synchronized void setMBeanServerForwarder(MBeanServerForwarder mbsf)
{
if (mbsf == null)
throw new IllegalArgumentException("Invalid null argument: mbsf");
if (mbeanServer != null) mbsf.setMBeanServer(mbeanServer);
mbeanServer = mbsf;
}
```
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #6770: GEODE-9515: Skip setting MBeanServer when JMXConnectorServer will set it
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6770:
URL: https://github.com/apache/geode/pull/6770#discussion_r692347003
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
##########
@@ -508,6 +517,10 @@ public JMXConnectorServer getJmxConnectorServer() {
return jmxConnectorServer;
}
+ public void setJmxConnectorServer(JMXConnectorServer jmxConnectorServer) {
+ this.jmxConnectorServer = jmxConnectorServer;
+ }
+
Review comment:
Should this method be `private` instead of `public`?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 merged pull request #6770: GEODE-9515: Skip setting MBeanServer when JMXConnectorServer will set it
Posted by GitBox <gi...@apache.org>.
jchen21 merged pull request #6770:
URL: https://github.com/apache/geode/pull/6770
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #6770: GEODE-9515: Skip setting MBeanServer when JMXConnectorServer will set it
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #6770:
URL: https://github.com/apache/geode/pull/6770#discussion_r692345016
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
##########
@@ -449,20 +463,15 @@ public synchronized void start() throws IOException {
if (accessFile != null && accessFile.length() > 0) {
// Rewire the mbs hierarchy to set accessController
ReadOpFileAccessController controller = new ReadOpFileAccessController(accessFile);
- controller.setMBeanServer(mbs);
+ if (jmxConnectorServer.getMBeanServer() == null) {
Review comment:
Looks like besides re-arranging code, this is the only effective change here. Do we know whey in this case this mbeanServer is not null?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #6770: GEODE-9515: Skip setting MBeanServer when JMXConnectorServer will set it
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #6770:
URL: https://github.com/apache/geode/pull/6770#discussion_r692345016
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
##########
@@ -449,20 +463,15 @@ public synchronized void start() throws IOException {
if (accessFile != null && accessFile.length() > 0) {
// Rewire the mbs hierarchy to set accessController
ReadOpFileAccessController controller = new ReadOpFileAccessController(accessFile);
- controller.setMBeanServer(mbs);
+ if (jmxConnectorServer.getMBeanServer() == null) {
Review comment:
Looks like besides re-arranging code, this is the only effective change here. Do we know why in this case this mbeanServer is not null?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #6770: GEODE-9515: Skip setting MBeanServer when JMXConnectorServer will set it
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6770:
URL: https://github.com/apache/geode/pull/6770#discussion_r692336257
##########
File path: geode-core/src/test/java/org/apache/geode/management/internal/ManagementAgentTest.java
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.geode.management.internal;
+
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.File;
+import java.io.IOException;
+import java.lang.management.ManagementFactory;
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.management.MBeanServer;
+import javax.management.remote.JMXConnectorServer;
+import javax.management.remote.JMXServiceURL;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import org.apache.geode.distributed.internal.DistributionConfig;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.security.SecurityService;
+
+public class ManagementAgentTest {
+
+ // private ManagementAgent managementAgent = spy(ManagementAgent.class);
+ @Rule
+ public TemporaryFolder temporaryFolder = new TemporaryFolder();
Review comment:
Let's move this test to `src/integrationTest` and rename to `ManagementAgentIntegrationTest` since it's touching the file system.
##########
File path: geode-core/src/test/java/org/apache/geode/management/internal/ManagementAgentTest.java
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.geode.management.internal;
+
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.File;
+import java.io.IOException;
+import java.lang.management.ManagementFactory;
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.management.MBeanServer;
+import javax.management.remote.JMXConnectorServer;
+import javax.management.remote.JMXServiceURL;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import org.apache.geode.distributed.internal.DistributionConfig;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.security.SecurityService;
+
+public class ManagementAgentTest {
+
+ // private ManagementAgent managementAgent = spy(ManagementAgent.class);
+ @Rule
+ public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+ @Test
+ public void testSetMBeanServer() throws IOException {
+ DistributionConfig distributionConfig = mock(DistributionConfig.class);
+ InternalCache internalCache = mock(InternalCache.class);
+ JmxRmiSerialFilter serialFilter = mock(JmxRmiSerialFilter.class);
+ SecurityService securityService = mock(SecurityService.class);
+ when(internalCache.getSecurityService()).thenReturn(securityService);
+ when(securityService.isIntegratedSecurity()).thenReturn(false);
+ File tempFile = temporaryFolder.newFile("testFile");
+ when(distributionConfig.getJmxManagerAccessFile()).thenReturn(tempFile.getCanonicalPath());
+ ManagementAgent managementAgent =
+ new ManagementAgent(distributionConfig, internalCache, serialFilter);
+ MBeanServer mBeanServer = mock(MBeanServer.class);
+ JMXConnectorServer jmxConnectorServerWithMBeanServer = new JMXConnectorServer(mBeanServer) {
Review comment:
I think you should move this to a static inner-class or better yet use Mockito to mock it:
```
JMXConnectorServer jmxConnectorServerWithMBeanServer = mock(JMXConnectorServer.class);
```
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #6770: GEODE-9515: Skip setting MBeanServer when JMXConnectorServer will set it
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #6770:
URL: https://github.com/apache/geode/pull/6770#discussion_r693102183
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
##########
@@ -449,20 +463,15 @@ public synchronized void start() throws IOException {
if (accessFile != null && accessFile.length() > 0) {
// Rewire the mbs hierarchy to set accessController
ReadOpFileAccessController controller = new ReadOpFileAccessController(accessFile);
- controller.setMBeanServer(mbs);
+ if (jmxConnectorServer.getMBeanServer() == null) {
Review comment:
I looked at the history of the file, the call `jmxConnectorServer.setMBeanServerForwarder(controller);` was added recently. When that call is added, we don't need the previous line `controller.setMBeanServer(mbs);` anymore.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org