You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by jujoramos <gi...@git.apache.org> on 2017/07/28 09:58:06 UTC
[GitHub] geode pull request #664: Fix for GEODE-3292
GitHub user jujoramos opened a pull request:
https://github.com/apache/geode/pull/664
Fix for GEODE-3292
Embedded PULSE Connection Failure When jmx-manager-bind-address != localhost
- ManagementAgent sets the 'pulse.host' system property whenever a 'jmx-manager-bind-address' is configured.
- PULSE gets the hostname of the local JMX Manager through the 'pulse.host' system property, reverting to 'localhost' by default.
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:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [X] Is your initial contribution a single, squashed commit?
- [X] Does `gradlew build` run cleanly?
- [X] 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, you check travis-ci 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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jujoramos/geode feature/GEODE-3292
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode/pull/664.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #664
----
commit 0c3e15f619ddd08f02ba8e2a8818f22d308c8bde
Author: Juan Jose Ramos Cassella <ju...@gmail.com>
Date: 2017-07-28T09:55:20Z
GEODE-3292: Embedded PULSE Connection Failure When jmx-manager-bind-address != localhost
- ManagementAgent sets the 'pulse.host' system property whenever a 'jmx-manager-bind-address' is configured.
- PULSE gets the hostname of the local JMX Manager through the 'pulse.host' system property, reverting to 'localhost' by default.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by jujoramos <gi...@git.apache.org>.
Github user jujoramos commented on a diff in the pull request:
https://github.com/apache/geode/pull/664#discussion_r131378087
--- Diff: geode-assembly/src/test/java/org/apache/geode/tools/pulse/AbstractPulseConnectivityTest.java ---
@@ -15,26 +15,31 @@
package org.apache.geode.tools.pulse;
+import static org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
import static org.assertj.core.api.Assertions.assertThat;
-import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
-import org.apache.geode.test.dunit.rules.HttpClientRule;
-import org.apache.geode.test.dunit.rules.LocatorStarterRule;
-import org.apache.geode.test.junit.categories.IntegrationTest;
-import org.apache.geode.tools.pulse.internal.data.Cluster;
+import java.io.File;
+import java.net.InetAddress;
+import java.util.Properties;
+
import org.apache.http.HttpResponse;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.junit.experimental.categories.Category;
+import org.junit.BeforeClass;
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.HttpClientRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
-@Category(IntegrationTest.class)
-public class PulseVerificationTest {
+public abstract class AbstractPulseConnectivityTest {
--- End diff --
Thanks for the tips, I couldn't find anything related to this intention within
[Writing Tests](https://cwiki.apache.org/confluence/display/GEODE/Writing+tests) but I'm okay anyway.
I'll give it one more try in my next commit later today, change things as you please :-).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/664#discussion_r131050061
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java ---
@@ -270,6 +271,7 @@ private void startHttpService(boolean isServer) {
}
System.setProperty(PULSE_EMBEDDED_PROP, "true");
+ System.setProperty(PULSE_HOST_PROP, "" + config.getJmxManagerBindAddress());
--- End diff --
+1 the product code change is good to go. Just some test re-org is needed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/664#discussion_r131182711
--- Diff: geode-pulse/src/test/java/org/apache/geode/tools/pulse/internal/PulseAppListenerTest.java ---
@@ -0,0 +1,91 @@
+/*
+ * 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.tools.pulse.internal;
+
+import javax.servlet.ServletContextEvent;
+import static org.mockito.Mockito.*;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TestRule;
+
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.apache.geode.tools.pulse.internal.data.PulseConstants;
+import org.apache.geode.tools.pulse.internal.data.Repository;
+
+@Category(UnitTest.class)
+public class PulseAppListenerTest {
+ private Repository repository;
+ private PulseAppListener appListener;
+
+ @Rule
+ public final TestRule restoreSystemProperties = new RestoreSystemProperties();
+
+ @Before
+ public void setUp() {
+ repository = Repository.get();
+ appListener = new PulseAppListener();
+ }
+
+ @Test
+ public void embeddedModeDefaultPropertiesRepositoryInitializationTest() {
+ System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_EMBEDDED, "true");
+ appListener.contextInitialized(mock(ServletContextEvent.class));
+
+ Assert.assertEquals(false, repository.getJmxUseLocator());
+ Assert.assertEquals(false, repository.isUseSSLManager());
+ Assert.assertEquals(false, repository.isUseSSLLocator());
+ Assert.assertEquals(PulseConstants.GEMFIRE_DEFAULT_PORT, repository.getPort());
+ Assert.assertEquals(PulseConstants.GEMFIRE_DEFAULT_HOST, repository.getHost());
+
+ }
+
+ @Test
+ public void embeddedModeNonDefaultPropertiesRepositoryInitializationTest() {
+ System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_EMBEDDED, "true");
+ System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_PORT, "9999");
+ System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_HOST, "nonDefaultBindAddress");
+ System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_USESSL_MANAGER,
+ Boolean.TRUE.toString());
+ System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_USESSL_LOCATOR,
+ Boolean.TRUE.toString());
+
+ appListener.contextInitialized(mock(ServletContextEvent.class));
+
+ Assert.assertEquals(false, repository.getJmxUseLocator());
+ Assert.assertEquals(true, repository.isUseSSLManager());
+ Assert.assertEquals(true, repository.isUseSSLLocator());
+ Assert.assertEquals("9999", repository.getPort());
+ Assert.assertEquals("nonDefaultBindAddress", repository.getHost());
+ }
+
+ @After
+ public void tearDown() {
+ if (repository != null) {
--- End diff --
this looks familiar, can you use EmbeddedPulseRule for this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/664#discussion_r131049655
--- Diff: geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java ---
@@ -0,0 +1,110 @@
+/*
+ * 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.tools.pulse;
+
+import static org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
+@Category(IntegrationTest.class)
+public class PulseConnectivityTest {
+ @ClassRule
+ public static LocatorStarterRule locator = new LocatorStarterRule().withJMXManager();
+
+ // Test Connectivity for Default Configuration
+ @Category(IntegrationTest.class)
+ public static class DefaultBindAddressTest {
+ @Rule
+ public EmbeddedPulseRule pulse = new EmbeddedPulseRule();
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ locator.startLocator();
+ }
+
+ @Test
+ public void testConnectToJmx() throws Exception {
+ pulse.useJmxPort(locator.getJmxPort());
+ Cluster cluster = pulse.getRepository().getCluster("admin", null);
+ assertThat(cluster.isConnectedFlag()).isTrue();
+ assertThat(cluster.getServerCount()).isEqualTo(0);
+ }
+
+ @Test
+ public void testConnectToLocator() throws Exception {
+ pulse.useLocatorPort(locator.getPort());
+ Cluster cluster = pulse.getRepository().getCluster("admin", null);
+ assertThat(cluster.isConnectedFlag()).isTrue();
+ assertThat(cluster.getServerCount()).isEqualTo(0);
+ }
+
+ @AfterClass
+ public static void afterClass() throws Exception {
+ locator.after();
+ }
+ }
+
+ // GEODE-3292: Test Connectivity for Non Default Configuration
+ @Category(IntegrationTest.class)
+ public static class NonDefaultBindAddressTest {
+ public static String jmxBindAddress;
+
+ @Rule
+ public EmbeddedPulseRule pulse = new EmbeddedPulseRule();
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ // Make sure we use something different than "localhost" for this test.
+ jmxBindAddress = InetAddress.getLocalHost().getHostName();
+ if ("localhost".equals(jmxBindAddress)) {
+ jmxBindAddress = InetAddress.getLocalHost().getHostAddress();
+ }
+
+ Properties locatorProperties = new Properties();
+ locatorProperties.setProperty(JMX_MANAGER_BIND_ADDRESS, jmxBindAddress);
+ locator.withProperties(locatorProperties).startLocator();
+ }
+
+ @Test
+ public void testConnectToJmx() throws Exception {
+ pulse.useJmxManager(jmxBindAddress, locator.getJmxPort());
+ Cluster cluster = pulse.getRepository().getCluster("admin", null);
+ assertThat(cluster.isConnectedFlag()).isTrue();
+ assertThat(cluster.getServerCount()).isEqualTo(0);
+ }
+
+ @AfterClass
+ public static void afterClass() throws Exception {
--- End diff --
Not needed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by jujoramos <gi...@git.apache.org>.
Github user jujoramos commented on a diff in the pull request:
https://github.com/apache/geode/pull/664#discussion_r131071385
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java ---
@@ -270,6 +271,7 @@ private void startHttpService(boolean isServer) {
}
System.setProperty(PULSE_EMBEDDED_PROP, "true");
+ System.setProperty(PULSE_HOST_PROP, "" + config.getJmxManagerBindAddress());
--- End diff --
I'll rewrite the tests and push the changes again, thanks for the suggestions!.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/664#discussion_r131050219
--- Diff: geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java ---
@@ -0,0 +1,110 @@
+/*
+ * 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.tools.pulse;
+
+import static org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
--- End diff --
the test you removed is slightly different from the one in PulseSecurityTest, there it's using a securityManager, but here, it doesn't, it's making sure the default username/password (admin/admin) works.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by jujoramos <gi...@git.apache.org>.
Github user jujoramos commented on a diff in the pull request:
https://github.com/apache/geode/pull/664#discussion_r131071365
--- Diff: geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java ---
@@ -0,0 +1,110 @@
+/*
+ * 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.tools.pulse;
+
+import static org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
+@Category(IntegrationTest.class)
+public class PulseConnectivityTest {
+ @ClassRule
+ public static LocatorStarterRule locator = new LocatorStarterRule().withJMXManager();
+
+ // Test Connectivity for Default Configuration
+ @Category(IntegrationTest.class)
+ public static class DefaultBindAddressTest {
+ @Rule
+ public EmbeddedPulseRule pulse = new EmbeddedPulseRule();
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
--- End diff --
True, but I'm not using `withAutoStart()` for a good reason: the two inner classes need to setup the locator differently (the `jmx-manager-bind-address` value changes).
I thought it's easier to read a single functional test class annotated with `@RunWith(Enclosed.class)` internally split in different inner classes than write a new test class to test exactly the same but changing a single locator property.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #664: Fix for GEODE-3292
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on the issue:
https://github.com/apache/geode/pull/664
I will pull this in the moment I got a green pipeline
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #664: Fix for GEODE-3292
Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on the issue:
https://github.com/apache/geode/pull/664
Thanks @jujoramos, this looks good to me. Do you have any thoughts @jinmeiliao?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #664: Fix for GEODE-3292
Posted by jujoramos <gi...@git.apache.org>.
Github user jujoramos commented on the issue:
https://github.com/apache/geode/pull/664
@jinmeiliao: thanks for reviewing this :-).
One more quick question and I'll leave you alone: do I need to change the status of [GEODE-3292](https://issues.apache.org/jira/browse/GEODE-3292) to `RESOLVED`?. Or that's done when the pull request is merged into `develop`?. Couldn't find a clear answer from [Code Contributions](https://cwiki.apache.org/confluence/display/GEODE/Code+contributions) and [Developer Workflow](https://cwiki.apache.org/confluence/display/GEODE/Developer+Workflow).
Cheers.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #664: Fix for GEODE-3292
Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on the issue:
https://github.com/apache/geode/pull/664
Hi Juan,
Thanks for your contribution! Your changes look good to me, but our [Criteria for Code Submissions](https://cwiki.apache.org/confluence/display/GEODE/Criteria+for+Code+Submissions) require having tests for the behavior that is being changed. In this case, it would be good to add a test to make sure that GEODE-3292 has been fixed and will not reoccur. Typically I would point you towards an existing test to use as a guide, but since your changes appear to be in an area of the code that is not particularly well-tested, let me know if you don't feel confident in writing such a test and I can help.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/664#discussion_r131049994
--- Diff: geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/EmbeddedPulseRule.java ---
@@ -38,6 +38,12 @@ protected void before() throws Throwable {
repository.setHost("localhost");
}
+ public void useJmxManager(String jmxHost, int jmxPort) {
--- End diff --
+1, very nice addition
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/geode/pull/664
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/664#discussion_r131182576
--- Diff: geode-assembly/src/test/java/org/apache/geode/tools/pulse/AbstractPulseConnectivityTest.java ---
@@ -15,26 +15,31 @@
package org.apache.geode.tools.pulse;
+import static org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
import static org.assertj.core.api.Assertions.assertThat;
-import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
-import org.apache.geode.test.dunit.rules.HttpClientRule;
-import org.apache.geode.test.dunit.rules.LocatorStarterRule;
-import org.apache.geode.test.junit.categories.IntegrationTest;
-import org.apache.geode.tools.pulse.internal.data.Cluster;
+import java.io.File;
+import java.net.InetAddress;
+import java.util.Properties;
+
import org.apache.http.HttpResponse;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.junit.experimental.categories.Category;
+import org.junit.BeforeClass;
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.HttpClientRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
-@Category(IntegrationTest.class)
-public class PulseVerificationTest {
+public abstract class AbstractPulseConnectivityTest {
--- End diff --
We would like to get rid of the usage of abstract test classes in favor of rules and parameterized tests. I see further improvements on these tests. But if you don't mind, I can pull your changes in and make modifications on top of yours.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by jujoramos <gi...@git.apache.org>.
Github user jujoramos commented on a diff in the pull request:
https://github.com/apache/geode/pull/664#discussion_r131367740
--- Diff: geode-pulse/src/test/java/org/apache/geode/tools/pulse/internal/PulseAppListenerTest.java ---
@@ -0,0 +1,91 @@
+/*
+ * 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.tools.pulse.internal;
+
+import javax.servlet.ServletContextEvent;
+import static org.mockito.Mockito.*;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TestRule;
+
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.apache.geode.tools.pulse.internal.data.PulseConstants;
+import org.apache.geode.tools.pulse.internal.data.Repository;
+
+@Category(UnitTest.class)
+public class PulseAppListenerTest {
+ private Repository repository;
+ private PulseAppListener appListener;
+
+ @Rule
+ public final TestRule restoreSystemProperties = new RestoreSystemProperties();
+
+ @Before
+ public void setUp() {
+ repository = Repository.get();
+ appListener = new PulseAppListener();
+ }
+
+ @Test
+ public void embeddedModeDefaultPropertiesRepositoryInitializationTest() {
+ System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_EMBEDDED, "true");
+ appListener.contextInitialized(mock(ServletContextEvent.class));
+
+ Assert.assertEquals(false, repository.getJmxUseLocator());
+ Assert.assertEquals(false, repository.isUseSSLManager());
+ Assert.assertEquals(false, repository.isUseSSLLocator());
+ Assert.assertEquals(PulseConstants.GEMFIRE_DEFAULT_PORT, repository.getPort());
+ Assert.assertEquals(PulseConstants.GEMFIRE_DEFAULT_HOST, repository.getHost());
+
+ }
+
+ @Test
+ public void embeddedModeNonDefaultPropertiesRepositoryInitializationTest() {
+ System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_EMBEDDED, "true");
+ System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_PORT, "9999");
+ System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_HOST, "nonDefaultBindAddress");
+ System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_USESSL_MANAGER,
+ Boolean.TRUE.toString());
+ System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_USESSL_LOCATOR,
+ Boolean.TRUE.toString());
+
+ appListener.contextInitialized(mock(ServletContextEvent.class));
+
+ Assert.assertEquals(false, repository.getJmxUseLocator());
+ Assert.assertEquals(true, repository.isUseSSLManager());
+ Assert.assertEquals(true, repository.isUseSSLLocator());
+ Assert.assertEquals("9999", repository.getPort());
+ Assert.assertEquals("nonDefaultBindAddress", repository.getHost());
+ }
+
+ @After
+ public void tearDown() {
+ if (repository != null) {
--- End diff --
Yes, I thought about using the existing rule but it implies adding a dependency from `geode-web` to `geode-assembly` as the rule is defined there, or duplicating the rule source code, that's why I didn't do it. Which approach would be best?.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #664: Fix for GEODE-3292
Posted by jujoramos <gi...@git.apache.org>.
Github user jujoramos commented on the issue:
https://github.com/apache/geode/pull/664
Hello @jaredjstewart,
Just pushed the requested changes.
- Renamed `PulseVerificationTest` to `PulseConnectivityTest`.
- Removed `loginWithIncorrectPassword` test method, it is the same as the one in `PulseSecurityTest`.
- Added integration tests to check the connectivity between PULSE and the local jmx-manager, both when the default and non-default bind-address is used.
- Added `PulseAppListenerTest` unit test to verify that the repository is correctly configured by the `PulseAppListener` when PULSE is running in embedded mode.
Cheers.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/664#discussion_r131049401
--- Diff: geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java ---
@@ -0,0 +1,110 @@
+/*
+ * 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.tools.pulse;
+
+import static org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
--- End diff --
I am not sure I've seen this pattern before. It looks like you are adding another set of test to the original PulseVerificationTest. I believe another separate test class would read better. I would leave the original test alone (since it has a test that uses httpClient to try to connect with a wrong password).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/664#discussion_r131049464
--- Diff: geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java ---
@@ -0,0 +1,110 @@
+/*
+ * 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.tools.pulse;
+
+import static org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
+@Category(IntegrationTest.class)
+public class PulseConnectivityTest {
+ @ClassRule
+ public static LocatorStarterRule locator = new LocatorStarterRule().withJMXManager();
+
+ // Test Connectivity for Default Configuration
+ @Category(IntegrationTest.class)
+ public static class DefaultBindAddressTest {
+ @Rule
+ public EmbeddedPulseRule pulse = new EmbeddedPulseRule();
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
--- End diff --
if you use new LocatorStarterRule().withAutoStart(), you don't need this @BeforeClass block anymore.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/664#discussion_r131049554
--- Diff: geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java ---
@@ -0,0 +1,110 @@
+/*
+ * 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.tools.pulse;
+
+import static org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
+@Category(IntegrationTest.class)
+public class PulseConnectivityTest {
+ @ClassRule
+ public static LocatorStarterRule locator = new LocatorStarterRule().withJMXManager();
+
+ // Test Connectivity for Default Configuration
+ @Category(IntegrationTest.class)
+ public static class DefaultBindAddressTest {
+ @Rule
+ public EmbeddedPulseRule pulse = new EmbeddedPulseRule();
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ locator.startLocator();
+ }
+
+ @Test
+ public void testConnectToJmx() throws Exception {
+ pulse.useJmxPort(locator.getJmxPort());
+ Cluster cluster = pulse.getRepository().getCluster("admin", null);
+ assertThat(cluster.isConnectedFlag()).isTrue();
+ assertThat(cluster.getServerCount()).isEqualTo(0);
+ }
+
+ @Test
+ public void testConnectToLocator() throws Exception {
+ pulse.useLocatorPort(locator.getPort());
+ Cluster cluster = pulse.getRepository().getCluster("admin", null);
+ assertThat(cluster.isConnectedFlag()).isTrue();
+ assertThat(cluster.getServerCount()).isEqualTo(0);
+ }
+
+ @AfterClass
+ public static void afterClass() throws Exception {
--- End diff --
this is not needed. The rule guarantees that this is called.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
Posted by jujoramos <gi...@git.apache.org>.
Github user jujoramos commented on a diff in the pull request:
https://github.com/apache/geode/pull/664#discussion_r131071353
--- Diff: geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java ---
@@ -0,0 +1,110 @@
+/*
+ * 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.tools.pulse;
+
+import static org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
--- End diff --
Yup, you're right, I didn't see the security manager within the rule and just looked at the method implementation. The login test doesn't seem to be functionality related to the connectivity test, that's why I removed it completely and aggregated the tests in a single class with two inner classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #664: Fix for GEODE-3292
Posted by jujoramos <gi...@git.apache.org>.
Github user jujoramos commented on the issue:
https://github.com/apache/geode/pull/664
Hey @jaredjstewart,
Thanks for looking at this.
I'll write the tests, no worries at all.
Cheers.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---