[GitHub] geode pull request #753: GEODE-3283: Expose parallel import and export in gf...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/753#discussion_r136638437
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ImportDataFunction.java
---
@@ -40,9 +40,13 @@ public void execute(FunctionContext context) {
final Object[] args = (Object[]) context.getArguments();
final String regionName = (String) args[0];
final String importFileName = (String) args[1];
-boolean invokeCallbacks = false;
+boolean parallel = false;
if (args.length > 2) {
--- End diff --
is the args length check necessary? would caller present different args at
different times?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #753: GEODE-3283: Expose parallel import and export in gf...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/753#discussion_r136639140
--- Diff:
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ImportDataIntegrationTest.java
---
@@ -0,0 +1,158 @@
+/*
+ * 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.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.Assert.assertEquals;
+
+import java.util.stream.IntStream;
+
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+
+@Category(IntegrationTest.class)
+public class ImportDataIntegrationTest extends SnapshotDataIntegrationTest
{
--- End diff --
In general we would want to discourage usage of abstract test classes and
in favor of rules. If there is minimum set up needed using the rules, we don't
mind a few lines of duplicate code for set up to avoid entangling of the tests.
What if in the future you want to change the setup of one test instead of
another. Extending a common abstract class makes it harder to add on to the
test.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #753: GEODE-3283: Expose parallel import and export in gf...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/753#discussion_r136638047
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportDataCommand.java
---
@@ -41,44 +44,34 @@ public Result exportData(
@CliOption(key = CliStrings.EXPORT_DATA__REGION, mandatory = true,
optionContext = ConverterHint.REGION_PATH,
help = CliStrings.EXPORT_DATA__REGION__HELP) String regionName,
- @CliOption(key = CliStrings.EXPORT_DATA__FILE, mandatory = true,
+ @CliOption(key = CliStrings.EXPORT_DATA__FILE,
help = CliStrings.EXPORT_DATA__FILE__HELP) String filePath,
+ @CliOption(key = CliStrings.EXPORT_DATA__DIR,
+ help = CliStrings.EXPORT_DATA__DIR__HELP) String dirPath,
@CliOption(key = CliStrings.MEMBER, optionContext =
ConverterHint.MEMBERIDNAME,
- mandatory = true, help = CliStrings.EXPORT_DATA__MEMBER__HELP)
String memberNameOrId) {
+ mandatory = true, help = CliStrings.EXPORT_DATA__MEMBER__HELP)
String memberNameOrId,
+ @CliOption(key = CliStrings.EXPORT_DATA__PARALLEL,
unspecifiedDefaultValue = "false",
--- End diff --
also, since there is a unspecified default value (false), is it really
necessary to make it a mandatory option if "dir" is specified? can we just make
it default to false(or true, whatever sensible) if user didn't add --parallel,
but specified --dir?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #753: GEODE-3283: Expose parallel import and export in gf...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/753#discussion_r136638633
--- Diff:
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportDataIntegrationTest.java
---
@@ -0,0 +1,127 @@
+/*
+ * 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.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.nio.file.Files;
+
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+
+@Category(IntegrationTest.class)
+public class ExportDataIntegrationTest extends SnapshotDataIntegrationTest
{
+ @Test
+ public void testExport() throws Exception {
+String exportCommand = buildBaseExportCommand()
+.addOption(CliStrings.EXPORT_DATA__FILE,
getSnapshotFile().toString()).getCommandString();
+gfsh.executeCommand(exportCommand);
--- End diff --
recommend using executeCommandAndVerify
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #753: GEODE-3283: Expose parallel import and export in gf...
Github user jinmeiliao commented on a diff in the pull request: https://github.com/apache/geode/pull/753#discussion_r136637196 --- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java --- @@ -1351,6 +1351,12 @@ "File to which the exported data will be written. The file must have an extension of \".gfd\"."; public static final String EXPORT_DATA__MEMBER__HELP = "Name/Id of a member which hosts the region. The data will be exported to the specified file on the host where the member is running."; + public static final String EXPORT_DATA__DIR = "directory"; --- End diff -- I think most commands will use "dir" instead of "directory" --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #753: GEODE-3283: Expose parallel import and export in gf...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/753#discussion_r136638194
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportDataCommand.java
---
@@ -87,4 +80,45 @@ public Result exportData(
}
return result;
}
+
+ private Result getFunctionResult(ResultCollector rc) {
+Result result;
+List results = (List) rc.getResult();
+if (results != null) {
+ Object resultObj = results.get(0);
+ if (resultObj instanceof String) {
+result = ResultBuilder.createInfoResult((String) resultObj);
+ } else if (resultObj instanceof Exception) {
+result = ResultBuilder.createGemFireErrorResult(((Exception)
resultObj).getMessage());
+ } else {
+result = ResultBuilder.createGemFireErrorResult(
+CliStrings.format(CliStrings.COMMAND_FAILURE_MESSAGE,
CliStrings.EXPORT_DATA));
+ }
+} else {
+ result = ResultBuilder.createGemFireErrorResult(
+ CliStrings.format(CliStrings.COMMAND_FAILURE_MESSAGE,
CliStrings.EXPORT_DATA));
+}
+return result;
+ }
+
+ private String defaultFileName(String dirPath, String regionName) {
+return new File(dirPath, regionName +
RegionSnapshotService.SNAPSHOT_FILE_EXTENSION)
+.getAbsolutePath();
+ }
+
+ private Optional validatePath(String filePath, String dirPath,
boolean parallel) {
--- End diff --
this seems to be duplicated between import/export, any way we can use the
same code base?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #753: GEODE-3283: Expose parallel import and export in gfsh
Github user jinmeiliao commented on the issue: https://github.com/apache/geode/pull/753 I would like to get a better understanding of the new feature first. So before the "export data" is export data of a specific region on a specific member to a file. All three options are mandatory. Now it seems I can export data of all regions on a specific member, so I should be able to do this: gfsh> export data --member=abc --dir=xxx Would the existence of --dir option implicitly mean "--parallel" already? do we need --parallel at all? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #745: GEODE-3436: Restore reverted GFSH command refactori...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/745#discussion_r135635795
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShowMetricsCommand.java
---
@@ -794,7 +84,7 @@ public Result showMetrics(
}
if (regionName != null && !regionName.isEmpty()) {
-if
(!org.apache.geode.internal.lang.StringUtils.isBlank(cacheServerPortString)) {
+if (!StringUtils.isBlank(cacheServerPortString)) {
--- End diff --
Let's leave these things for other cleanup tasks. The focus of this change
set is to break these command into separate classes. I know we all can't resist
the urge of making everything perfect from time to time, but one thing at a
time.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #664: Fix for GEODE-3292
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #664: Fix for GEODE-3292
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
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, "");
+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("", 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #664: Fix for GEODE-3292
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #664: Fix for GEODE-3292
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #664: Fix for GEODE-3292
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #660: GEODE-2924 Revise authorization permissions
Github user jinmeiliao commented on a diff in the pull request: https://github.com/apache/geode/pull/660#discussion_r129910320 --- Diff: geode-docs/managing/security/implementing_authorization.html.md.erb --- @@ -56,13 +56,23 @@ which classifies whether the operation as The operations are not hierarchical; `MANAGE` does not imply `WRITE`, and `WRITE` does not imply `READ`. -Some operations further specify a region name in the permission. +Some `DATA` operations further specify a region name in the permission. This permits restricting operations on that region to only those authorized principals. And within a region, some operations may specify a key. This permits restricting operations on that key within that region to only those authorized principals. +Some `CLUSTER` operations further specify a finer-grained +target for the operation. +Specify the target with a string value of: + +- `DISK` to target operations that write to a disk store +- `GATEWAY` to target operations that manage gateway senders and receivers +- `QUERY` to target operations that manage both indexes and continuous + queries +- `JAR` to target operations that deploy code to servers + --- End diff -- I believe there are more changes to the permission strings than just these few here. We also made some corrections like: echo: N/A encrypt password: N/A (actually encrypt password is no longer a gfsh command anymore, we removed it). execute function: determined by function api. A lot of the GatewayMXBean operation are changed as well. Please go through the list of "new permission strings in https://cwiki.apache.org/confluence/display/GEODE/Finer+grained+security and make all the modifications 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #660: GEODE-2924 Revise authorization permissions
Github user jinmeiliao commented on a diff in the pull request: https://github.com/apache/geode/pull/660#discussion_r129908926 --- Diff: geode-docs/managing/security/implementing_authorization.html.md.erb --- @@ -56,13 +56,23 @@ which classifies whether the operation as The operations are not hierarchical; `MANAGE` does not imply `WRITE`, and `WRITE` does not imply `READ`. -Some operations further specify a region name in the permission. +Some `DATA` operations further specify a region name in the permission. This permits restricting operations on that region to only those authorized principals. And within a region, some operations may specify a key. This permits restricting operations on that key within that region to only those authorized principals. +Some `CLUSTER` operations further specify a finer-grained +target for the operation. +Specify the target with a string value of: + +- `DISK` to target operations that write to a disk store +- `GATEWAY` to target operations that manage gateway senders and receivers +- `QUERY` to target operations that manage both indexes and continuous + queries +- `JAR` to target operations that deploy code to servers + --- End diff -- there is "LUCENE" as well for lucene queries --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129673457
--- Diff: geode-core/src/main/java/org/apache/geode/internal/ExitCode.java
---
@@ -0,0 +1,61 @@
+/*
+ * 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.internal;
+
+import java.util.Arrays;
+
+import org.springframework.shell.core.ExitShellRequest;
+
+
+public enum ExitCode {
+
+ // The choice of redundant values here is to be consistent with existing
behavior
+ // while allowing for future extensibility. JVM_TERMINATED_EXIT(99)
exists for coverage of
+ // Spring's ExitShellRequest values in fromSpring.
+ NORMAL(0),
+ FATAL(1),
+ DEPENDENCY_GRAPH_FAILURE(-1),
+ COULD_NOT_EXECUTE_COMMAND(1),
+ INVALID_COMMAND(1),
--- End diff --
It's the exit code that matters. As long as you are not changing the code
that each command gives when it's calling system.exit(), you are not changing
existing behavior. Or Am I missing anything here?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129647221
--- Diff:
geode-assembly/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExitCodeStatusCommandsDUnitTest.java
---
@@ -0,0 +1,396 @@
+/*
+ * 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.cli.shell;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.internal.AvailablePort;
+import org.apache.geode.internal.ExitCode;
+import org.apache.geode.internal.process.PidFile;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.management.internal.cli.util.ThreePhraseGenerator;
+import org.apache.geode.test.dunit.rules.gfsh.GfshExecution;
+import org.apache.geode.test.dunit.rules.gfsh.GfshRule;
+import org.apache.geode.test.dunit.rules.gfsh.GfshScript;
+import org.apache.geode.test.junit.categories.AcceptanceTest;
+
+// Originally created in response to GEODE-2971
+
+@Category(AcceptanceTest.class)
+@RunWith(JUnitParamsRunner.class)
+public class GfshExitCodeStatusCommandsDUnitTest {
+ private static File toolsJar;
+ private final static ThreePhraseGenerator nameGenerator = new
ThreePhraseGenerator();
+ private final static String memberControllerName = "member-controller";
+
+ @Rule
+ public GfshRule gfsh = new GfshRule();
+ private String locatorName;
+ private String serverName;
+
+ private int locatorPort;
+
+ // Some test configuration shorthands
+ private TestConfiguration LOCATOR_ONLINE_BUT_NOT_CONNECTED =
+ new TestConfiguration(true, false, false);
+ private TestConfiguration LOCATOR_ONLINE_AND_CONNECTED = new
TestConfiguration(true, false, true);
+ private TestConfiguration BOTH_ONLINE_BUT_NOT_CONNECTED =
+ new TestConfiguration(true, true, false);
+ private TestConfiguration BOTH_ONLINE_AND_CONNECTED = new
TestConfiguration(true, true, true);
+
+ @BeforeClass
+ public static void classSetup() {
+File javaHome = new File(System.getProperty("java.home"));
+String toolsPath =
+javaHome.getName().equalsIgnoreCase("jre") ? "../lib/tools.jar" :
"lib/tools.jar";
+toolsJar = new File(javaHome, toolsPath);
+ }
+
+ @Before
+ public void setup() {
+locatorName = "locator-" + nameGenerator.generate('-');
+serverName = "server-" + nameGenerator.generate('-');
+locatorPort =
AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
+ }
+
+ @Test
+ @Parameters(
+ value = {"status locator --port=-10", "status locator --pid=-1",
"status server --pid=-1"})
+ public void statusCommandWithInvalidOptionValueShouldFail(String cmd) {
+GfshScript.of(cmd).withName("test-frame").awaitAtMost(1, MINUTES)
+.expectExitCode(ExitCode.FATAL.getValue()).execute(gfsh);
+ }
+
+
+ @Test
+ @Parameters(value = {"status locator --host=somehostname", "status
locator --port=10334",
+ "status locator --dir=.", "status server --dir=.", "status locator
--name=some-locator-name",
+ "status server --name=some-server-name", "status locator --pid=-1",
"status server --pid=-1"})
+ public void
statusCommandWithValidOptionValueShouldFailWithNoMembers(
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129646761
--- Diff: geode-core/src/main/java/org/apache/geode/internal/ExitCode.java
---
@@ -0,0 +1,61 @@
+/*
+ * 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.internal;
+
+import java.util.Arrays;
+
+import org.springframework.shell.core.ExitShellRequest;
+
+
+public enum ExitCode {
+
+ // The choice of redundant values here is to be consistent with existing
behavior
+ // while allowing for future extensibility. JVM_TERMINATED_EXIT(99)
exists for coverage of
+ // Spring's ExitShellRequest values in fromSpring.
+ NORMAL(0),
+ FATAL(1),
+ DEPENDENCY_GRAPH_FAILURE(-1),
+ COULD_NOT_EXECUTE_COMMAND(1),
+ INVALID_COMMAND(1),
--- End diff --
I would think for each enum, the value passed to it would be distinct.
What's the reason for multiple enum value have the same shellExitCode? Would
this be more concise?
enum ExitCode{
DEPENDENCY_GRAPH_FAILURE(-1),
NORMAL(0),
FATAL(1),
INSTALL_FAILURE(2),
}
I am hesitant to put the exit code used only for test in here. But that's
debatable.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #598: GEODE-2818: Completing implementation/testing of al...
Github user jinmeiliao commented on a diff in the pull request: https://github.com/apache/geode/pull/598#discussion_r128354651 --- Diff: geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/OptionAliasesIntegrationTest.java --- @@ -0,0 +1,259 @@ +/* + * 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.cli.commands; --- End diff -- these tests simply checks the exit code for each command, and not checking if the server really belongs to these groups. I am wondering do we really need such a long running tests to test the command options. I would think that OptionAliasParserTest would be sufficient. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #590: GEODE-3090: Fixing gfsh help message (and a lot of other t...
Github user jinmeiliao commented on the issue: https://github.com/apache/geode/pull/590 pulling this in. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #557: add GfshParserRule to facilitate command unit test
Github user jinmeiliao closed the pull request at: https://github.com/apache/geode/pull/557 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #604: GEODE-2919: fix lucene security tests
Github user jinmeiliao closed the pull request at: https://github.com/apache/geode/pull/604 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #608: GEODE-3140: Removed DiskDirRule and replaced use wi...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/608#discussion_r124434616
--- Diff:
geode-lucene/src/test/java/org/apache/geode/cache/lucene/LuceneQueriesPersistenceIntegrationTest.java
---
@@ -48,13 +49,13 @@
public class LuceneQueriesPersistenceIntegrationTest extends
LuceneIntegrationTest {
@Rule
- public DiskDirRule diskDirRule = new DiskDirRule();
+ public TemporaryFolder tempFolderRule = new TemporaryFolder(new
File("."));
--- End diff --
Is there any reason we want to pass in a parentFolder of current dir? Can
we just use the temp folder that we don't care about where it is?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #604: GEODE-2919: fix lucene security tests
GitHub user jinmeiliao opened a pull request: https://github.com/apache/geode/pull/604 GEODE-2919: fix lucene security tests this is to fix the failing lucent tests. Created a PR to see how travis thinks about it. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jinmeiliao/geode fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/604.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 #604 commit 4d1f3bf71dd0eefced4a2fae76af03a7c3e91b72 Author: Jinmei Liao <jil...@pivotal.io> Date: 2017-06-27T00:34:20Z GEODE-2919: fix lucene security tests --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #589: GEODE-393: Providing cache for FunctionContext
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/589#discussion_r123845237
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/execute/FunctionContextImpl.java
---
@@ -37,20 +38,25 @@
private String functionId = null;
+ private Cache cache = null;
+
private ResultSender resultSender = null;
private final boolean isPossDup;
public FunctionContextImpl(final String functionId, final Object args,
ResultSender resultSender) {
-this.functionId = functionId;
-this.args = args;
-this.resultSender = resultSender;
-this.isPossDup = false;
+this(null, functionId, args, resultSender, false);
+ }
+
+ public FunctionContextImpl(final Cache cache, final String functionId,
final Object args,
+ ResultSender resultSender) {
+this(cache, functionId, args, resultSender, false);
}
- public FunctionContextImpl(final String functionId, final Object args,
ResultSender resultSender,
- boolean isPossibleDuplicate) {
+ public FunctionContextImpl(final Cache cache, final String functionId,
final Object args,
+ ResultSender resultSender, boolean isPossibleDuplicate) {
--- End diff --
I agree, once we get rid of that static gemfire cache function, these
changes will be necessary. better start making the changes somewhere.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #589: GEODE-393: Providing cache for FunctionContext
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/589#discussion_r123818259
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/execute/FunctionContextImpl.java
---
@@ -37,20 +38,25 @@
private String functionId = null;
+ private Cache cache = null;
+
private ResultSender resultSender = null;
private final boolean isPossDup;
public FunctionContextImpl(final String functionId, final Object args,
ResultSender resultSender) {
-this.functionId = functionId;
-this.args = args;
-this.resultSender = resultSender;
-this.isPossDup = false;
+this(null, functionId, args, resultSender, false);
+ }
+
+ public FunctionContextImpl(final Cache cache, final String functionId,
final Object args,
+ ResultSender resultSender) {
+this(cache, functionId, args, resultSender, false);
}
- public FunctionContextImpl(final String functionId, final Object args,
ResultSender resultSender,
- boolean isPossibleDuplicate) {
+ public FunctionContextImpl(final Cache cache, final String functionId,
final Object args,
+ ResultSender resultSender, boolean isPossibleDuplicate) {
--- End diff --
we can't remove GemFireCacheImpl.getInstance() for now, that's the way we
are getting the instance to create the FunctionContext in the first place, why
not just use it to avoid making more constructor changes. This just my 2 cents.
I can go either way on this though.
The problem this changeset is trying to solve is that if external developer
can not use GemfireCacheImpl.getInstance() to get the cache since it's an
internal api, they can only use CacheFactory.getAnyIntance() and that's a
synchronized static, so it requires a lock on CachFactory which creates some
deadlocks.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #589: GEODE-393: Providing cache for FunctionContext
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/589#discussion_r123544960
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/execute/FunctionContextImpl.java
---
@@ -37,20 +38,25 @@
private String functionId = null;
+ private Cache cache = null;
+
private ResultSender resultSender = null;
private final boolean isPossDup;
public FunctionContextImpl(final String functionId, final Object args,
ResultSender resultSender) {
-this.functionId = functionId;
-this.args = args;
-this.resultSender = resultSender;
-this.isPossDup = false;
+this(null, functionId, args, resultSender, false);
+ }
+
+ public FunctionContextImpl(final Cache cache, final String functionId,
final Object args,
+ ResultSender resultSender) {
+this(cache, functionId, args, resultSender, false);
}
- public FunctionContextImpl(final String functionId, final Object args,
ResultSender resultSender,
- boolean isPossibleDuplicate) {
+ public FunctionContextImpl(final Cache cache, final String functionId,
final Object args,
+ ResultSender resultSender, boolean isPossibleDuplicate) {
--- End diff --
or better yet, use GemfireCacheImpl.getExisting() instead of getInstance
because the first one provide checking for existing cache.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #589: GEODE-393: Providing cache for FunctionContext
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/589#discussion_r123544587
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/execute/FunctionContextImpl.java
---
@@ -37,20 +38,25 @@
private String functionId = null;
+ private Cache cache = null;
+
private ResultSender resultSender = null;
private final boolean isPossDup;
public FunctionContextImpl(final String functionId, final Object args,
ResultSender resultSender) {
-this.functionId = functionId;
-this.args = args;
-this.resultSender = resultSender;
-this.isPossDup = false;
+this(null, functionId, args, resultSender, false);
+ }
+
+ public FunctionContextImpl(final Cache cache, final String functionId,
final Object args,
+ ResultSender resultSender) {
+this(cache, functionId, args, resultSender, false);
}
- public FunctionContextImpl(final String functionId, final Object args,
ResultSender resultSender,
- boolean isPossibleDuplicate) {
+ public FunctionContextImpl(final Cache cache, final String functionId,
final Object args,
+ ResultSender resultSender, boolean isPossibleDuplicate) {
--- End diff --
Looking at the places where it's grabbing the cache to create the
FunctionContext, it's all using GemfireCacheImpl.getInstance(). I am wondering
we should just have the implementation of getCache() in FunctionContextImpl to
be simply GemfireCacheImpl.getInstance(), then we don't need to change all the
constructor signatures and keep a member variable of the cache in the context.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #589: GEODE-393: Providing cache for FunctionContext
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/589#discussion_r123332996
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/execute/FunctionContextImpl.java
---
@@ -37,20 +38,25 @@
private String functionId = null;
+ private Cache cache = null;
+
private ResultSender resultSender = null;
private final boolean isPossDup;
public FunctionContextImpl(final String functionId, final Object args,
ResultSender resultSender) {
-this.functionId = functionId;
-this.args = args;
-this.resultSender = resultSender;
-this.isPossDup = false;
+this(null, functionId, args, resultSender, false);
+ }
+
+ public FunctionContextImpl(final Cache cache, final String functionId,
final Object args,
+ ResultSender resultSender) {
+this(cache, functionId, args, resultSender, false);
}
- public FunctionContextImpl(final String functionId, final Object args,
ResultSender resultSender,
- boolean isPossibleDuplicate) {
+ public FunctionContextImpl(final Cache cache, final String functionId,
final Object args,
+ ResultSender resultSender, boolean isPossibleDuplicate) {
--- End diff --
this could still have have cache end up being null if user did not create
the FunctionContextImpl with a cache, either pass in null, or use the other
non-cache constructor. It would be good to add a check here:
if(cache==null){
cache = GemfireCacheImpl.getExisting();
}
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #548: GEODE-2818: add alias to any command's options that...
Github user jinmeiliao commented on a diff in the pull request: https://github.com/apache/geode/pull/548#discussion_r120134823 --- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommands.java --- @@ -412,7 +411,7 @@ public Result compactDiskStore( @CliOption(key = CliStrings.COMPACT_DISK_STORE__NAME, mandatory = true, optionContext = ConverterHint.DISKSTORE, help = CliStrings.COMPACT_DISK_STORE__NAME__HELP) String diskStoreName, - @CliOption(key = CliStrings.COMPACT_DISK_STORE__GROUP, + @CliOption(key = CliStrings.GROUP, --- End diff -- looks like this can have "groups" as synonym as well. Can you go through all your changes and make sure that member/group CliOption that take String[] as the option type needs to have synonym defined. Thanks! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #557: add GfshParserRule to facilitate command unit test
GitHub user jinmeiliao opened a pull request: https://github.com/apache/geode/pull/557 add GfshParserRule to facilitate command unit test 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, 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/jinmeiliao/geode gfshParserRule Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/557.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 #557 commit 7e66a6eae6ede08280711d0112f6c5afae198d1d Author: Jinmei Liao <jil...@pivotal.io> Date: 2017-06-05T06:51:42Z add GfshParserRule to fascilidate command unit test --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #530: GEODE-2981: Fix the line feed code of the test expected va...
Github user jinmeiliao commented on the issue: https://github.com/apache/geode/pull/530 will pull this in after testing --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #546: GEODE-2818: add alias to any command's options that...
Github user jinmeiliao commented on a diff in the pull request: https://github.com/apache/geode/pull/546#discussion_r119159305 --- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java --- @@ -726,13 +733,13 @@ public static final String CREATE_INDEX__REGION = "region"; public static final String CREATE_INDEX__REGION__HELP = "Name/Path of the region which corresponds to the \"from\" clause in a query."; - public static final String CREATE_INDEX__MEMBER = "member"; + public static final String CREATE_INDEX__MEMBER = MEMBER; --- End diff -- I am wondering if you can delete all these other xxx_GROUP or xxx_MEMBER string declarations and change all reference of them to use the generic ones. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #500: GEODE-2662: Gfsh displays field value on wrong line...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/500#discussion_r117080378
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java
---
@@ -406,124 +400,140 @@ public Result toCommandResult() {
section.addData("Message", errorString);
section.addData(RESULT_FLAG, operationCompletedSuccessfully);
return ResultBuilder.buildResult(data);
+}
+
+CompositeResultData data = ResultBuilder.createCompositeResultData();
+SectionResultData section = data.addSection();
+TabularResultData table = section.addTable();
+
+section.addData(RESULT_FLAG, operationCompletedSuccessfully);
+if (infoString != null) {
+ section.addData("Message", infoString);
+}
+
+if (isGet()) {
+ toCommandResult_isGet(section, table);
+} else if (isLocateEntry()) {
+ toCommandResult_isLocate(section, table);
+} else if (isPut()) {
+ toCommandResult_isPut(section, table);
+} else if (isRemove()) {
+ toCommandResult_isRemove(section, table);
+} else if (isSelect()) {
+ // its moved to its separate method
--- End diff --
so, for isSelect() we don't do anything? looks like isSelect() is only used
here, probably we can remove this function? looks like there is a lot of unused
functions in this class as well.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #500: GEODE-2662: Gfsh displays field value on wrong line...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/500#discussion_r117081706
--- Diff:
geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java
---
@@ -145,6 +153,7 @@ public void startServer(boolean pdxPersistent) {
CacheFactory cf = new CacheFactory(this.properties);
cf.setPdxReadSerialized(pdxPersistent);
cf.setPdxPersistent(pdxPersistent);
+this.pdxPersistent = pdxPersistent;
--- End diff --
I don't think we should add this line here.
we can get rid of this "public void startServer(boolean pdxPersistent)"
method now since we have withPdxPersistant added. the flag should solely be
determined by the value of the member variable.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #452: GEODE-2730: refactor rules
Github user jinmeiliao commented on the issue: https://github.com/apache/geode/pull/452 resort back to review board. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #452: GEODE-2730: refactor rules
Github user jinmeiliao closed the pull request at: https://github.com/apache/geode/pull/452 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #452: GEODE-2730: refactor rules
Github user jinmeiliao commented on the issue: https://github.com/apache/geode/pull/452 review board is having some trouble updating my old reviews. I am using the pull request. It should addressed all the points that Ken wrote in the old review. But, please review more. Thanks! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #452: GEODE-2730: refactor rules
GitHub user jinmeiliao opened a pull request: https://github.com/apache/geode/pull/452 GEODE-2730: refactor rules * consolidate the two sets of server/locator starter rules * do not allow member start up at test initialization time. * validate properties in @Before * use provider in the chained rules to get the appropriate ports in @Before You can merge this pull request into a Git repository by running: $ git pull https://github.com/jinmeiliao/geode rules Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/452.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 #452 commit 330911c38b445d53aba49688488c2e014e457ebd Author: Jinmei Liao <jil...@pivotal.io> Date: 2017-03-23T02:45:19Z GEODE-2730: refactor rules * consolidate the two sets of server/locator starter rules * do not allow member start up at test initialization time. * validate properties in @Before * use provider in the chained rules to get the appropriate ports in @Before --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #445: GEODE-2767: Added DUnitTests to validate export log option...
Github user jinmeiliao commented on the issue: https://github.com/apache/geode/pull/445 I'll pull this in. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #438: GEODE-2725: export logs now honors --dir when connected vi...
Github user jinmeiliao commented on the issue: https://github.com/apache/geode/pull/438 I'll pull this in --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #439: GEODE-2716: export logs default behavior changed from filt...
Github user jinmeiliao commented on the issue: https://github.com/apache/geode/pull/439 I'll pull this in. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #432: GEODE-1274: Migration from PulseLogWriter to Log4j standar...
Github user jinmeiliao commented on the issue: https://github.com/apache/geode/pull/432 Is the comment on here https://reviews.apache.org/r/57822/ addressed in this PR? Is precheckin successful? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #429: Geode-2686
Github user jinmeiliao commented on the issue: https://github.com/apache/geode/pull/429 It would be a good idea to do a walkthrough of these changes for us to understand the major change points. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #335: Pulse fixes
Github user jinmeiliao commented on a diff in the pull request: https://github.com/apache/geode/pull/335#discussion_r95898738 --- Diff: geode-pulse/src/main/webapp/WEB-INF/web.xml --- @@ -32,7 +32,7 @@ mvc-dispatcher -/pulse/* --- End diff -- i see. if changing it back to /pulse/* makes no difference, then add it back for clarify. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #335: Pulse fixes
Github user jinmeiliao commented on a diff in the pull request: https://github.com/apache/geode/pull/335#discussion_r95892864 --- Diff: geode-pulse/src/main/webapp/WEB-INF/web.xml --- @@ -32,7 +32,7 @@ mvc-dispatcher -/pulse/* --- End diff -- This seems dangerous. Is this ok because we are already deploying the war under /pulse? Have you tested starting pulse and connect to gfsh using http? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #328: [GEODE-2138] #comment Fix issues GEODE-2138
Github user jinmeiliao commented on the issue: https://github.com/apache/geode/pull/328 I'll pull this in. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #327: GEODE-2236: Remove all cache-listners using Gfsh
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/327#discussion_r95418869
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/RegionAlterFunction.java
---
@@ -237,7 +237,7 @@ public void execute(FunctionContext context) {
}
}
-if (!nameFound) {
+if (!nameFound && !newCacheListenerName.isEmpty()) {
--- End diff --
instead of adding the check here, would it be better to add the check in
line after 232:
if(newCacheListnerName.isEmpty()) break;
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #327: GEODE-2236: Remove all cache-listners using Gfsh
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/327#discussion_r95420817
--- Diff:
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommandsDUnitTest.java
---
@@ -833,6 +833,120 @@ private void regionAlterManipulatePlugInsTest() {
});
}
+ @Test
+ public void testAlterRegionResetCacheListeners() throws IOException {
+setUpJmxManagerOnVm0ThenConnect(null);
+
+CommandResult cmdResult = executeCommand(CliStrings.LIST_REGION);
+assertEquals(Result.Status.OK, cmdResult.getStatus());
+assertTrue(commandResultToString(cmdResult).contains("No Regions
Found"));
+
+Host.getHost(0).getVM(0).invoke(() -> {
+ Cache cache = getCache();
+
cache.createRegionFactory(RegionShortcut.PARTITION).setStatisticsEnabled(true)
+ .create(alterRegionName);
+});
+
+this.alterVm1 = Host.getHost(0).getVM(1);
+this.alterVm1Name = "VM" + this.alterVm1.getPid();
+this.alterVm1.invoke(() -> {
+ Properties localProps = new Properties();
+ localProps.setProperty(NAME, alterVm1Name);
+ localProps.setProperty(GROUPS, "Group1");
+ getSystem(localProps);
+ Cache cache = getCache();
+
+ // Setup queues and gateway senders to be used by all tests
+
cache.createRegionFactory(RegionShortcut.PARTITION).setStatisticsEnabled(true)
+ .create(alterRegionName);
+ AsyncEventListener listener = new AsyncEventListener() {
--- End diff --
The EventListener is unnecessary for this test.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #327: GEODE-2236: Remove all cache-listners using Gfsh
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/327#discussion_r95420944
--- Diff:
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommandsDUnitTest.java
---
@@ -833,6 +833,120 @@ private void regionAlterManipulatePlugInsTest() {
});
}
+ @Test
+ public void testAlterRegionResetCacheListeners() throws IOException {
+setUpJmxManagerOnVm0ThenConnect(null);
+
+CommandResult cmdResult = executeCommand(CliStrings.LIST_REGION);
+assertEquals(Result.Status.OK, cmdResult.getStatus());
+assertTrue(commandResultToString(cmdResult).contains("No Regions
Found"));
+
+Host.getHost(0).getVM(0).invoke(() -> {
+ Cache cache = getCache();
+
cache.createRegionFactory(RegionShortcut.PARTITION).setStatisticsEnabled(true)
+ .create(alterRegionName);
+});
+
+this.alterVm1 = Host.getHost(0).getVM(1);
+this.alterVm1Name = "VM" + this.alterVm1.getPid();
+this.alterVm1.invoke(() -> {
+ Properties localProps = new Properties();
+ localProps.setProperty(NAME, alterVm1Name);
+ localProps.setProperty(GROUPS, "Group1");
+ getSystem(localProps);
+ Cache cache = getCache();
+
+ // Setup queues and gateway senders to be used by all tests
+
cache.createRegionFactory(RegionShortcut.PARTITION).setStatisticsEnabled(true)
+ .create(alterRegionName);
+ AsyncEventListener listener = new AsyncEventListener() {
+@Override
+public void close() {
+ // Nothing to do
+}
+
+@Override
+public boolean processEvents(List events) {
+ return true;
+}
+ };
+});
+
+this.alterVm2 = Host.getHost(0).getVM(2);
+this.alterVm2Name = "VM" + this.alterVm2.getPid();
+this.alterVm2.invoke(() -> {
+ Properties localProps = new Properties();
+ localProps.setProperty(NAME, alterVm2Name);
+ localProps.setProperty(GROUPS, "Group1,Group2");
+ getSystem(localProps);
+ Cache cache = getCache();
+
+
cache.createRegionFactory(RegionShortcut.PARTITION).setStatisticsEnabled(true)
+ .create(alterRegionName);
+});
+
+deployJarFilesForRegionAlter();
+regionAlterResetCacheListenersTest();
--- End diff --
since this method is only called by this test, move the body of the
function to this test and delete the private function.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #308: [GEODE-2167] BundledJarsJUnitTest fails on Windows
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/308#discussion_r93131489
--- Diff: geode-core/src/test/java/org/apache/geode/util/test/TestUtil.java
---
@@ -46,11 +47,15 @@ public static String getResourcePath(Class clazz,
String name) {
File tmpFile = File.createTempFile(filename, null);
tmpFile.deleteOnExit();
FileUtil.copy(resource, tmpFile);
-return tmpFile.getAbsolutePath();
+return compatibleWithWindows(tmpFile.getAbsolutePath());
}
- return path;
+ return compatibleWithWindows(path);
} catch (URISyntaxException | IOException e) {
throw new RuntimeException("Failed getting path to resource " +
name, e);
}
}
+
+ private static String compatibleWithWindows(String path) {
--- End diff --
maybe make it public and call it getPath(File file)?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #316: GEODE-2188: ExampleSecurityManager references Sampl...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/316#discussion_r93097202
--- Diff:
geode-core/src/main/java/org/apache/geode/examples/security/ExampleSecurityManager.java
---
@@ -257,15 +257,65 @@ private void readUsers(final Map<String, User>
rolesToUsers, final JsonNode node
return roleMap;
}
- static class Role {
+ public static class Role {
List permissions = new ArrayList<>();
+
--- End diff --
why was it working before? Was it because it was in the same package? If
your test code is testing ExampleSecurityManager, then better just leave it
public and have the getter/setters there.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #316: GEODE-2188: ExampleSecurityManager references Sampl...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/316#discussion_r92888056
--- Diff:
geode-core/src/main/java/org/apache/geode/examples/security/ExampleSecurityManager.java
---
@@ -257,15 +257,65 @@ private void readUsers(final Map<String, User>
rolesToUsers, final JsonNode node
return roleMap;
}
- static class Role {
+ public static class Role {
List permissions = new ArrayList<>();
+
--- End diff --
I meant those getters and setters inside class User and class Role. Those
were not there before your change set unless I am missing something.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #317: [GEODE-2196] Add test for exportClusterConfig.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/317#discussion_r92666840
--- Diff:
geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
---
@@ -85,21 +91,16 @@ public void connect(String... options) throws Exception
{
// javax.naming.CommunicationException [Root exception is
java.rmi.NoSuchObjectException: no
// such object in table]" Exception.
// Tried to wait on jmx connector server being ready, but it doesn't
work.
-// Add the retry logic here to try at most 10 times for connection.
-CommandResult result = null;
-for (int i = 0; i < 50; i++) {
- System.out.println("trying to connect, attempt " + i);
+Awaitility.await().atMost(2, TimeUnit.MINUTES).pollDelay(2,
TimeUnit.SECONDS).until(() -> {
gfsh.executeCommand(connectCommand.toString());
- result = (CommandResult) gfsh.getResult();
- System.out.println(gfsh.outputString);
- if (!gfsh.outputString.contains("no such object in table")) {
-break;
- }
- Thread.currentThread().sleep(2000);
-}
+ return !gfsh.outputString.contains("no such object in table");
+});
+
+CommandResult result = (CommandResult) gfsh.getResult();
--- End diff --
the getResult call needs to be inside the awaitility loop. The result is
put in a queue everytime a command is called. If you only call it outside the
loop, the result you got is the first command result in the loop.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #317: [GEODE-2196] Add test for exportClusterConfig.
Github user jinmeiliao commented on a diff in the pull request: https://github.com/apache/geode/pull/317#discussion_r92544631 --- Diff: geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java --- @@ -44,32 +46,32 @@ /** * This is only available in each Locator/Server VM, not in the controller (test) VM. */ - public static ServerStarterRule serverStarter; + public static transient ServerStarterRule serverStarter; --- End diff -- I think all static variables are transient 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #317: [GEODE-2196] Add test for exportClusterConfig.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/317#discussion_r92544756
--- Diff:
geode-core/src/test/java/org/apache/geode/test/dunit/rules/Locator.java ---
@@ -0,0 +1,28 @@
+/*
+ * 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.test.dunit.rules;
+
+import org.apache.geode.test.dunit.VM;
+
+import java.io.File;
+
+public class Locator extends Member {
--- End diff --
Why are we not using Member for both locator/server? The child class does
not provide anything else the abstract class doesn't do.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #317: [GEODE-2196] Add test for exportClusterConfig.
Github user jinmeiliao commented on a diff in the pull request: https://github.com/apache/geode/pull/317#discussion_r92544538 --- Diff: geode-core/src/test/java/org/apache/geode/test/dunit/rules/ClusterConfig.java --- @@ -0,0 +1,183 @@ +/* + * 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.test.dunit.rules; --- End diff -- Do we want to put this in the rules package or should we moved into the cluster config test package? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #317: [GEODE-2196] Add test for exportClusterConfig.
Github user jinmeiliao commented on a diff in the pull request: https://github.com/apache/geode/pull/317#discussion_r92544596 --- Diff: geode-core/src/test/java/org/apache/geode/test/dunit/rules/ConfigGroup.java --- @@ -0,0 +1,102 @@ +/* + * 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 + * thisright 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 + * this 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.test.dunit.rules; --- End diff -- Same as before, probably move to another more specific package --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #317: [GEODE-2196] Add test for exportClusterConfig.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/317#discussion_r92545081
--- Diff:
geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigImportDUnitTest.java
---
@@ -0,0 +1,144 @@
+/*
+ * 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.configuration;
+
+import static org.apache.geode.distributed.ConfigurationProperties.GROUPS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.internal.cli.result.CommandResult;
+import org.apache.geode.test.dunit.rules.ClusterConfig;
+import org.apache.geode.test.dunit.rules.ConfigGroup;
+import org.apache.geode.test.dunit.rules.GfshShellConnectionRule;
+import org.apache.geode.test.dunit.rules.Locator;
+import org.apache.geode.test.dunit.rules.Server;
+import org.apache.geode.test.junit.categories.DistributedTest;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.File;
+import java.nio.file.Path;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipFile;
+
+@Category(DistributedTest.class)
+public class ClusterConfigImportDUnitTest extends ClusterConfigBaseTest {
+ private GfshShellConnectionRule gfshConnector;
+
+ public static final ClusterConfig INITIAL_CONFIG = new ClusterConfig(new
ConfigGroup("cluster"));
+
+ private Locator locator;
+
+ @Before
+ public void before() throws Exception {
+super.before();
+locator = lsRule.startLocatorVM(0, locatorProps);
+INITIAL_CONFIG.verify(locator);
+
+gfshConnector = new GfshShellConnectionRule(locator);
+gfshConnector.connect();
+assertThat(gfshConnector.isConnected()).isTrue();
+ }
+
+
+ @After
+ public void after() throws Exception {
+if (gfshConnector != null) {
+ gfshConnector.close();
+}
+ }
+
+ @Test
+ public void testImportWithRunningServer() throws Exception {
+Server server1 = lsRule.startServerVM(1, serverProps,
locator.getPort());
+
+CommandResult result = gfshConnector.executeCommand(
+"import cluster-configuration --zip-file-name=" +
EXPORTED_CLUSTER_CONFIG_PATH);
+
+assertThat(result.getStatus()).isEqualTo(Result.Status.ERROR);
+ }
+
+ @Test
+ public void testImportClusterConfig() throws Exception {
+CommandResult result = gfshConnector.executeCommand(
+"import cluster-configuration --zip-file-name=" +
EXPORTED_CLUSTER_CONFIG_PATH);
+assertThat(result.getStatus()).isEqualTo(Result.Status.OK);
+
+// verify that the previous folder is backed up to
"cluster_configxx".
+assertThat(locator.getWorkingDir().listFiles())
+.filteredOn((File file) ->
!file.getName().equals("cluster_config"))
+.filteredOn((File file) ->
file.getName().startsWith("cluster_config")).isNotEmpty();
+CONFIG_FROM_ZIP.verify(locator);
+
+// start server1 with no group
+Server server1 = lsRule.startServerVM(1, serverProps,
locator.getPort());
+new ClusterConfig(CLUSTER).verify(server1);
+
+// start server2 in group1
+serverProps.setProperty(GROUPS, "group1");
+Server server2 = lsRule.startServerVM(2, serverProps,
locator.getPort());
+new ClusterConfig(CLUSTER, GROUP1).verify(server2);
+
+// start server3 in group1 and group2
+serverProps.setProperty(GROUPS, "group1,group2");
+Server server3 = lsRule.startServerVM(3, serverProps,
locator.getPort());
+new ClusterConfig(CLUSTER, GROUP1, GROUP2).
[GitHub] geode pull request #316: GEODE-2188: ExampleSecurityManager references Sampl...
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/316#discussion_r92544045
--- Diff:
geode-core/src/main/java/org/apache/geode/examples/security/ExampleSecurityManager.java
---
@@ -257,15 +257,65 @@ private void readUsers(final Map<String, User>
rolesToUsers, final JsonNode node
return roleMap;
}
- static class Role {
+ public static class Role {
List permissions = new ArrayList<>();
+
--- End diff --
These methods are not used anywhere, why do we need to add them?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
