[GitHub] geode pull request #745: GEODE-3436: Restore reverted GFSH command refactori...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/745#discussion_r135639047
--- Diff:
geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java
---
@@ -171,28 +171,37 @@ private static void init() {
createTestCommand("alter disk-store --name=foo --region=xyz
--disk-dirs=bar");
createTestCommand("destroy disk-store --name=foo", clusterManageDisk);
-// DurableClientCommands
+// CloseDurableClientCommand, CloseDurableCQsCommand,
CountDurableCQEventsCommand,
+// ListDurableClientCQsCommand
createTestCommand("close durable-client --durable-client-id=client1",
clusterManageQuery);
createTestCommand("close durable-cq --durable-client-id=client1
--durable-cq-name=cq1",
clusterManageQuery);
createTestCommand("show subscription-queue-size
--durable-client-id=client1", clusterRead);
createTestCommand("list durable-cqs --durable-client-id=client1",
clusterRead);
-// ExportIMportSharedConfigurationCommands
+// ExportImportSharedConfigurationCommands
createTestCommand("export cluster-configuration
--zip-file-name=mySharedConfig.zip",
clusterRead);
createTestCommand("import cluster-configuration
--zip-file-name=value.zip", clusterManage);
-// FunctionCommands
+// DestroyFunctionCommand, ExecuteFunctionCommand, ListFunctionCommand
+// TODO PSR: the `destroy function` command is interactive (in its
interceptor) when both
--- End diff --
Have got a resolution on these TODO's?
---
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 pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/745#discussion_r135633281
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExecuteFunctionCommand.java
---
@@ -156,7 +132,7 @@ public Result executeFunction(
// if user wish to execute on locator then he can choose --member
or --group option
Set dsMembers =
CliUtil.getAllNormalMembers(cache);
if (dsMembers.size() > 0) {
- function = new UserFunctionExecution();
+ new UserFunctionExecution();
--- End diff --
Where is this object used?
---
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 pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/745#discussion_r135629055
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommand.java
---
@@ -247,7 +207,7 @@ public Result createRegion(
new Object[] {CliStrings.CREATE_REGION__USEATTRIBUTESFROM,
useAttributesFrom}));
--- End diff --
Bit of a nit-pick, but we have been trying to clean up these cases where we
unnecessarily create new object arrays to pass to varargs methods. This occurs
several places in the class
---
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 pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/745#discussion_r135597926
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateDefinedIndexesCommand.java
---
@@ -0,0 +1,155 @@
+/*
+ * 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 java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.springframework.shell.core.annotation.CliCommand;
+import org.springframework.shell.core.annotation.CliOption;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.management.cli.CliMetaData;
+import org.apache.geode.management.cli.ConverterHint;
+import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.internal.cli.CliUtil;
+import
org.apache.geode.management.internal.cli.functions.CliFunctionResult;
+import
org.apache.geode.management.internal.cli.functions.CreateDefinedIndexesFunction;
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.geode.management.internal.cli.result.ErrorResultData;
+import org.apache.geode.management.internal.cli.result.InfoResultData;
+import org.apache.geode.management.internal.cli.result.ResultBuilder;
+import org.apache.geode.management.internal.configuration.domain.XmlEntity;
+import org.apache.geode.management.internal.security.ResourceOperation;
+import org.apache.geode.security.ResourcePermission;
+
+public class CreateDefinedIndexesCommand implements GfshCommand {
+ private static final CreateDefinedIndexesFunction
createDefinedIndexesFunction =
+ new CreateDefinedIndexesFunction();
+
+ @CliCommand(value = CliStrings.CREATE_DEFINED_INDEXES, help =
CliStrings.CREATE_DEFINED__HELP)
+ @CliMetaData(relatedTopic = {CliStrings.TOPIC_GEODE_REGION,
CliStrings.TOPIC_GEODE_DATA})
+ @ResourceOperation(resource = ResourcePermission.Resource.CLUSTER,
+ operation = ResourcePermission.Operation.MANAGE, target =
ResourcePermission.Target.QUERY)
+ // TODO : Add optionContext for indexName
+ public Result createDefinedIndexes(
+
+ @CliOption(key = {CliStrings.MEMBER, CliStrings.MEMBERS},
+ optionContext = ConverterHint.MEMBERIDNAME,
+ help = CliStrings.CREATE_DEFINED_INDEXES__MEMBER__HELP) final
String[] memberNameOrID,
+
+ @CliOption(key = {CliStrings.GROUP, CliStrings.GROUPS},
+ optionContext = ConverterHint.MEMBERGROUP,
+ help = CliStrings.CREATE_DEFINED_INDEXES__GROUP__HELP) final
String[] group) {
+
+Result result;
+AtomicReference xmlEntity = new AtomicReference<>();
+
+if (IndexDefinition.indexDefinitions.isEmpty()) {
+ final InfoResultData infoResult =
ResultBuilder.createInfoResultData();
+ infoResult.addLine(CliStrings.DEFINE_INDEX__FAILURE__MSG);
+ return ResultBuilder.buildResult(infoResult);
+}
+
+try {
+ final Set targetMembers =
CliUtil.findMembers(group, memberNameOrID);
+
+ if (targetMembers.isEmpty()) {
+return
ResultBuilder.createUserErrorResult(CliStrings.NO_MEMBERS_FOUND_MESSAGE);
+ }
+
+ // TODO PSR: is this safe to remove?
+ CacheFactory.getAnyInstance();
--- End diff --
I think this should be replaced by calling the GfshCommand interface's
getCache() to limit the places where this potentially deadlock prone call is
made. . Most (or at least many) other gfsh commands call the interface's method.
---
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 fea
[GitHub] geode pull request #730: GEODE-3472: Remove a great deal of commented-out co...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/730#discussion_r134804772
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/result/AbstractResultData.java
---
@@ -147,11 +147,10 @@ public ResultData addAsFile(String fileName, String
fileContents, String message
public ResultData addAsFile(String fileName, byte[] data, int fileType,
String message,
boolean addTimeStampToName) {
-byte[] bytes = data;
--- End diff --
I stand corrected. I confused myself with past experiences with other
languages.
---
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 #730: GEODE-3472: Remove a great deal of commented-out co...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/730#discussion_r134616035
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/result/AbstractResultData.java
---
@@ -147,11 +147,10 @@ public ResultData addAsFile(String fileName, String
fileContents, String message
public ResultData addAsFile(String fileName, byte[] data, int fileType,
String message,
boolean addTimeStampToName) {
-byte[] bytes = data;
--- End diff --
I think the contents of `data` should be treated as if it were immutable in
this usage, so don't make this and the following change.
---
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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132483129
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java
---
@@ -14,745 +14,270 @@
*/
package org.apache.geode.distributed;
-import org.apache.geode.distributed.AbstractLauncher.Status;
+import static
org.apache.geode.distributed.AbstractLauncher.Status.NOT_RESPONDING;
+import static org.apache.geode.distributed.AbstractLauncher.Status.ONLINE;
+import static org.apache.geode.distributed.AbstractLauncher.Status.STOPPED;
+import static
org.apache.geode.distributed.ConfigurationProperties.DISABLE_AUTO_RECONNECT;
+import static
org.apache.geode.distributed.ConfigurationProperties.LOG_LEVEL;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static org.apache.geode.distributed.ConfigurationProperties.NAME;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.File;
+import java.net.BindException;
+import java.net.InetAddress;
+import java.util.Collections;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
import org.apache.geode.distributed.LocatorLauncher.Builder;
import org.apache.geode.distributed.LocatorLauncher.LocatorState;
import org.apache.geode.distributed.internal.InternalLocator;
-import org.apache.geode.internal.*;
-import org.apache.geode.internal.net.SocketCreatorFactory;
+import org.apache.geode.internal.GemFireVersion;
import org.apache.geode.internal.process.ProcessControllerFactory;
import org.apache.geode.internal.process.ProcessType;
-import org.apache.geode.internal.process.ProcessUtils;
-import org.apache.geode.internal.security.SecurableCommunicationChannel;
import org.apache.geode.test.junit.categories.IntegrationTest;
-import
org.apache.geode.test.junit.runners.CategoryWithParameterizedRunnerFactory;
-import org.junit.After;
-import org.junit.Before;
-import org.junit.Ignore;
-import org.junit.Test;
-import org.junit.experimental.categories.Category;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-import java.io.File;
-import java.lang.management.ManagementFactory;
-import java.net.BindException;
-import java.net.InetAddress;
-
-import static org.apache.geode.distributed.ConfigurationProperties.*;
-import static org.junit.Assert.*;
/**
- * Tests usage of LocatorLauncher as a local API in existing JVM.
+ * Integration tests for using {@link LocatorLauncher} as an in-process
API within an existing JVM.
*
* @since GemFire 8.0
*/
@Category(IntegrationTest.class)
-@RunWith(Parameterized.class)
-@Parameterized.UseParametersRunnerFactory(CategoryWithParameterizedRunnerFactory.class)
-public class LocatorLauncherLocalIntegrationTest
-extends AbstractLocatorLauncherIntegrationTestCase {
+public class LocatorLauncherLocalIntegrationTest extends
LocatorLauncherIntegrationTestCase {
@Before
- public final void setUpLocatorLauncherLocalIntegrationTest() throws
Exception {
+ public void setUpLocatorLauncherLocalIntegrationTest() throws Exception {
disconnectFromDS();
-System.setProperty(ProcessType.TEST_PREFIX_PROPERTY, getUniqueName() +
"-");
+System.setProperty(ProcessType.PROPERTY_TEST_PREFIX, getUniqueName() +
"-");
+assertThat(new ProcessControllerFactory().isAttachAPIFound()).isTrue();
}
@After
- public final void tearDownLocatorLauncherLocalIntegrationTest() throws
Exception {
+ public void tearDownLocatorLauncherLocalIntegrationTest() throws
Exception {
disconnectFromDS();
}
@Test
- public void testBuilderSetProperties() throws Throwable {
-this.launcher = new
Builder().setForce(true).setMemberName(getUniqueName())
-
.setPort(this.locatorPort).setWorkingDirectory(this.workingDirectory)
-.set(CLUSTER_CONFIGURATION_DIR, this.clusterConfigDirectory)
-.set(DISABLE_AUTO_RECONNECT, "true").set(LOG_LEVEL,
"config").set(MCAST_PORT, "0").build();
-
-try {
- assertEquals(Status.ONLINE, this.launcher.start().getStatus());
- waitForLocatorToStart(this.launcher, true);
-
- final InternalLocator locator = this.launcher.getLocator();
- assertNotNull(locator);
-
- final DistributedSystem distributedSystem =
locator.getDistributedSystem();
-
- assertNotNull(distributedSystem);
- assertEquals(&quo
[GitHub] geode pull request #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132515331
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java ---
@@ -810,9 +819,6 @@ public void setStatus(final String statusMessage) {
LocalizedStrings.Launcher_Command_START_PID_UNAVAILABLE_ERROR_MESSAGE.toLocalizedString(
getServiceName(), getId(), getWorkingDirectory(),
e.getMessage()),
e);
- } catch (ClusterConfigurationNotAvailableException e) {
-failOnStart(e);
-throw e;
} catch (RuntimeException e) {
failOnStart(e);
--- End diff --
Opportunity to combine catch blocks?
---
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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request: https://github.com/apache/geode/pull/699#discussion_r132520010 --- Diff: geode-core/src/main/java/org/apache/geode/internal/process/StartupStatus.java --- @@ -14,29 +14,37 @@ */ package org.apache.geode.internal.process; +import static org.apache.commons.lang.Validate.notNull; + import org.apache.logging.log4j.Logger; -import org.apache.geode.internal.logging.LogService; import org.apache.geode.i18n.StringId; +import org.apache.geode.internal.logging.LogService; /** * Extracted from LogWriterImpl and changed to static. - * */ --- End diff -- Is this javadoc necessary? --- 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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request: https://github.com/apache/geode/pull/699#discussion_r132501696 --- Diff: geode-core/src/test/java/org/apache/geode/internal/process/mbean/Process.java --- @@ -16,7 +16,6 @@ /** * Extracted from LocalProcessControllerDUnitTest. - * */ --- End diff -- This javadoc doesn't seem meaningful. --- 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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132497014
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/process/NonBlockingProcessStreamReaderIntegrationTest.java
---
@@ -0,0 +1,182 @@
+/*
+ * 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.process;
+
+import static
org.apache.geode.internal.process.ProcessStreamReader.ReadingMode.NON_BLOCKING;
+import static
org.apache.geode.internal.process.ProcessUtils.isProcessAlive;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.internal.process.ProcessStreamReader.Builder;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+
+/**
+ * Functional integration tests for NonBlockingProcessStreamReader which
was introduced to fix TRAC
+ * #51967.
+ *
+ * @see BlockingProcessStreamReaderIntegrationTest
+ * @see BlockingProcessStreamReaderWindowsTest
+ *
+ * @since GemFire 8.2
+ */
+@Category(IntegrationTest.class)
+public class NonBlockingProcessStreamReaderIntegrationTest
+extends BaseProcessStreamReaderIntegrationTest {
+
+ private StringBuffer stdoutBuffer;
+ private StringBuffer stderrBuffer;
+
+ @Before
+ public void before() {
+stdoutBuffer = new StringBuffer();
+stderrBuffer = new StringBuffer();
+ }
+
+ /**
+ * This test hangs on Windows if the implementation is blocking instead
of non-blocking.
+ */
+ @Test
+ public void canCloseStreamsWhileProcessIsAlive() throws Exception {
+// arrange
+process = new
ProcessBuilder(createCommandLine(ProcessSleeps.class)).start();
+stdout = new
Builder(process).inputStream(process.getInputStream()).readingMode(NON_BLOCKING)
+.build().start();
+stderr = new
Builder(process).inputStream(process.getErrorStream()).readingMode(NON_BLOCKING)
--- End diff --
It looks like there may be an opportunity to extract a method from the
common code from the beginning of each test. Yes, there are some variations on
setting stdout and stderr, so maybe a couple methods to avoid duplicated code?
---
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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132325049
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java ---
@@ -1075,8 +1082,7 @@ private LocatorState stopWithWorkingDirectory() {
try {
final ProcessController controller =
new
ProcessControllerFactory().createProcessController(this.controllerParameters,
- new File(getWorkingDirectory()),
ProcessType.LOCATOR.getPidFileName(),
- READ_PID_FILE_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS);
+ new File(getWorkingDirectory()),
ProcessType.LOCATOR.getPidFileName());
parsedPid = controller.getProcessId();
// NOTE in-process request will go infinite loop unless we do the
following
--- End diff --
There are several opportunities for collapsing multiple catch blocks in
this class, such as the try/catch that starts at line 1082
---
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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132303567
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/process/BlockingProcessStreamReaderWindowsTest.java
---
@@ -0,0 +1,97 @@
+/*
+ * 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.process;
+
+import static org.apache.geode.internal.lang.SystemUtils.isWindows;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.junit.Assume.assumeTrue;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.test.junit.categories.IntegrationTest;
+
+/**
+ * Functional integration test {@link #hangsOnWindows} for
BlockingProcessStreamReader which
+ * verifies TRAC #51967 on Windows. The hang is caused by a JDK bug in
which a thread invoking
--- End diff --
The bug number reference is not meaningful in the context of the geode
community. I suggest rewording this to remove the specific reference while
still describing the the nature of the bug that this test verifies the fix for.
---
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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132314371
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java
---
@@ -0,0 +1,505 @@
+/*
+ * 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.distributed;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static org.apache.geode.internal.AvailablePort.SOCKET;
+import static org.apache.geode.internal.AvailablePort.isPortAvailable;
+import static org.apache.geode.internal.process.ProcessUtils.identifyPid;
+import static
org.apache.geode.internal.process.ProcessUtils.isProcessAlive;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.Assert.assertTrue;
+
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.FileReader;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.lang.management.ManagementFactory;
+import java.net.ServerSocket;
+import java.nio.file.Files;
+import java.util.List;
+import java.util.Properties;
+import java.util.concurrent.Callable;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.logging.log4j.Logger;
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionFactory;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+import org.junit.rules.TemporaryFolder;
+import org.junit.rules.TestName;
+
+import org.apache.geode.distributed.internal.DistributionConfig;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.internal.logging.LogService;
+import org.apache.geode.internal.net.SocketCreatorFactory;
+import org.apache.geode.internal.process.PidUnavailableException;
+import org.apache.geode.internal.process.ProcessStreamReader.InputListener;
+import org.apache.geode.internal.process.ProcessType;
+import org.apache.geode.internal.process.ProcessUtils;
+import org.apache.geode.internal.process.lang.AvailablePid;
+import org.apache.geode.internal.util.StopWatch;
+import org.apache.geode.test.dunit.IgnoredException;
+
+/**
+ * Abstract base class for integration tests of both {@link
LocatorLauncher} and
+ * {@link ServerLauncher}.
+ *
+ * @since GemFire 8.0
+ */
+public abstract class LauncherIntegrationTestCase {
+ protected static final Logger logger = LogService.getLogger();
+
+ protected static final int WAIT_FOR_PROCESS_TO_DIE_TIMEOUT = 5 * 60 *
1000; // 5 minutes
+ protected static final int TIMEOUT_MILLISECONDS =
WAIT_FOR_PROCESS_TO_DIE_TIMEOUT;
+ protected static final int WAIT_FOR_FILE_CREATION_TIMEOUT = 10 * 1000;
// 10s
+ protected static final int WAIT_FOR_FILE_DELETION_TIMEOUT = 10 * 1000;
// 10s
+ protected static final int WAIT_FOR_MBEAN_TIMEOUT = 10 * 1000; // 10s
--- End diff --
Unused field?
---
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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132314473
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java
---
@@ -0,0 +1,505 @@
+/*
+ * 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.distributed;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static org.apache.geode.internal.AvailablePort.SOCKET;
+import static org.apache.geode.internal.AvailablePort.isPortAvailable;
+import static org.apache.geode.internal.process.ProcessUtils.identifyPid;
+import static
org.apache.geode.internal.process.ProcessUtils.isProcessAlive;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.Assert.assertTrue;
+
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.FileReader;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.lang.management.ManagementFactory;
+import java.net.ServerSocket;
+import java.nio.file.Files;
+import java.util.List;
+import java.util.Properties;
+import java.util.concurrent.Callable;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.logging.log4j.Logger;
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionFactory;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+import org.junit.rules.TemporaryFolder;
+import org.junit.rules.TestName;
+
+import org.apache.geode.distributed.internal.DistributionConfig;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.internal.logging.LogService;
+import org.apache.geode.internal.net.SocketCreatorFactory;
+import org.apache.geode.internal.process.PidUnavailableException;
+import org.apache.geode.internal.process.ProcessStreamReader.InputListener;
+import org.apache.geode.internal.process.ProcessType;
+import org.apache.geode.internal.process.ProcessUtils;
+import org.apache.geode.internal.process.lang.AvailablePid;
+import org.apache.geode.internal.util.StopWatch;
+import org.apache.geode.test.dunit.IgnoredException;
+
+/**
+ * Abstract base class for integration tests of both {@link
LocatorLauncher} and
+ * {@link ServerLauncher}.
+ *
+ * @since GemFire 8.0
+ */
+public abstract class LauncherIntegrationTestCase {
+ protected static final Logger logger = LogService.getLogger();
+
+ protected static final int WAIT_FOR_PROCESS_TO_DIE_TIMEOUT = 5 * 60 *
1000; // 5 minutes
+ protected static final int TIMEOUT_MILLISECONDS =
WAIT_FOR_PROCESS_TO_DIE_TIMEOUT;
+ protected static final int WAIT_FOR_FILE_CREATION_TIMEOUT = 10 * 1000;
// 10s
+ protected static final int WAIT_FOR_FILE_DELETION_TIMEOUT = 10 * 1000;
// 10s
+ protected static final int WAIT_FOR_MBEAN_TIMEOUT = 10 * 1000; // 10s
+ protected static final int INTERVAL_MILLISECONDS = 100;
+
+ protected static final int PREFERRED_FAKE_PID = 42;
+
+ private static final String EXPECTED_EXCEPTION_ADD =
+ "{}";
+ private static final String EXPECTED_EXCEPTION_REMOVE =
--- End diff --
Unused fields?
---
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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132315473
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java
---
@@ -0,0 +1,505 @@
+/*
+ * 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.distributed;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static org.apache.geode.internal.AvailablePort.SOCKET;
+import static org.apache.geode.internal.AvailablePort.isPortAvailable;
+import static org.apache.geode.internal.process.ProcessUtils.identifyPid;
+import static
org.apache.geode.internal.process.ProcessUtils.isProcessAlive;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.Assert.assertTrue;
+
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.FileReader;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.lang.management.ManagementFactory;
+import java.net.ServerSocket;
+import java.nio.file.Files;
+import java.util.List;
+import java.util.Properties;
+import java.util.concurrent.Callable;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.logging.log4j.Logger;
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionFactory;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+import org.junit.rules.TemporaryFolder;
+import org.junit.rules.TestName;
+
+import org.apache.geode.distributed.internal.DistributionConfig;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.internal.logging.LogService;
+import org.apache.geode.internal.net.SocketCreatorFactory;
+import org.apache.geode.internal.process.PidUnavailableException;
+import org.apache.geode.internal.process.ProcessStreamReader.InputListener;
+import org.apache.geode.internal.process.ProcessType;
+import org.apache.geode.internal.process.ProcessUtils;
+import org.apache.geode.internal.process.lang.AvailablePid;
+import org.apache.geode.internal.util.StopWatch;
+import org.apache.geode.test.dunit.IgnoredException;
+
+/**
+ * Abstract base class for integration tests of both {@link
LocatorLauncher} and
+ * {@link ServerLauncher}.
+ *
+ * @since GemFire 8.0
+ */
+public abstract class LauncherIntegrationTestCase {
+ protected static final Logger logger = LogService.getLogger();
+
+ protected static final int WAIT_FOR_PROCESS_TO_DIE_TIMEOUT = 5 * 60 *
1000; // 5 minutes
+ protected static final int TIMEOUT_MILLISECONDS =
WAIT_FOR_PROCESS_TO_DIE_TIMEOUT;
+ protected static final int WAIT_FOR_FILE_CREATION_TIMEOUT = 10 * 1000;
// 10s
+ protected static final int WAIT_FOR_FILE_DELETION_TIMEOUT = 10 * 1000;
// 10s
+ protected static final int WAIT_FOR_MBEAN_TIMEOUT = 10 * 1000; // 10s
+ protected static final int INTERVAL_MILLISECONDS = 100;
+
+ protected static final int PREFERRED_FAKE_PID = 42;
+
+ private static final String EXPECTED_EXCEPTION_ADD =
+ "{}";
+ private static final String EXPECTED_EXCEPTION_REMOVE =
+ "{}";
+ private static final String EXPECTED_EXCEPTION_MBEAN_NOT_REGISTERED =
+ "MBean Not Registered In GemFire Domain";
+
+ private IgnoredException ignoredException;
+
+ protected int localPid;
+ protected int fakePid;
+
+ protected volatile ServerSocket socket;
+
+ protected volatile File pidFile;
+ protected volatile File stopRequestFile;
+ protected volatile File statusRequestFile;
+ protected volatile File statusFile;
+
+ @Rule
+ public Test
[GitHub] geode pull request #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request: https://github.com/apache/geode/pull/699#discussion_r132304059 --- Diff: geode-core/src/test/java/org/apache/geode/internal/process/BlockingProcessStreamReaderIntegrationTest.java --- @@ -0,0 +1,179 @@ +/* + * 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.process; + +import static org.apache.geode.internal.lang.SystemUtils.isWindows; +import static org.apache.geode.internal.process.ProcessUtils.isProcessAlive; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assume.assumeFalse; + +import org.junit.Before; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import org.apache.geode.internal.process.ProcessStreamReader.Builder; +import org.apache.geode.internal.process.ProcessStreamReader.ReadingMode; +import org.apache.geode.test.junit.categories.IntegrationTest; + +/** + * Functional integration tests for BlockingProcessStreamReader. All tests are skipped on Windows + * due to TRAC bug #51967 which is caused by a JDK bug. See --- End diff -- See my other comment regarding specific bug reference. --- 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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132314337
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java
---
@@ -0,0 +1,505 @@
+/*
+ * 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.distributed;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static org.apache.geode.internal.AvailablePort.SOCKET;
+import static org.apache.geode.internal.AvailablePort.isPortAvailable;
+import static org.apache.geode.internal.process.ProcessUtils.identifyPid;
+import static
org.apache.geode.internal.process.ProcessUtils.isProcessAlive;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.Assert.assertTrue;
+
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.FileReader;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.lang.management.ManagementFactory;
+import java.net.ServerSocket;
+import java.nio.file.Files;
+import java.util.List;
+import java.util.Properties;
+import java.util.concurrent.Callable;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.logging.log4j.Logger;
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionFactory;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+import org.junit.rules.TemporaryFolder;
+import org.junit.rules.TestName;
+
+import org.apache.geode.distributed.internal.DistributionConfig;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.internal.logging.LogService;
+import org.apache.geode.internal.net.SocketCreatorFactory;
+import org.apache.geode.internal.process.PidUnavailableException;
+import org.apache.geode.internal.process.ProcessStreamReader.InputListener;
+import org.apache.geode.internal.process.ProcessType;
+import org.apache.geode.internal.process.ProcessUtils;
+import org.apache.geode.internal.process.lang.AvailablePid;
+import org.apache.geode.internal.util.StopWatch;
+import org.apache.geode.test.dunit.IgnoredException;
+
+/**
+ * Abstract base class for integration tests of both {@link
LocatorLauncher} and
+ * {@link ServerLauncher}.
+ *
+ * @since GemFire 8.0
+ */
+public abstract class LauncherIntegrationTestCase {
+ protected static final Logger logger = LogService.getLogger();
+
+ protected static final int WAIT_FOR_PROCESS_TO_DIE_TIMEOUT = 5 * 60 *
1000; // 5 minutes
+ protected static final int TIMEOUT_MILLISECONDS =
WAIT_FOR_PROCESS_TO_DIE_TIMEOUT;
--- End diff --
Unused field?
---
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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request: https://github.com/apache/geode/pull/699#discussion_r132301005 --- Diff: geode-core/src/test/java/org/apache/geode/internal/process/lang/AvailablePidTest.java --- @@ -0,0 +1,74 @@ +/* + * 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.process.lang; --- End diff -- Ideally there would be a test for 'findAvailablePid(existing processPid)' returns a random pid. --- 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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132285291
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/AbstractLauncherTest.java
---
@@ -14,253 +14,370 @@
*/
package org.apache.geode.distributed;
+import static java.util.concurrent.TimeUnit.DAYS;
+import static java.util.concurrent.TimeUnit.HOURS;
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.MINUTES;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static
org.apache.geode.distributed.AbstractLauncher.ServiceState.toDaysHoursMinutesSeconds;
import static org.apache.geode.distributed.ConfigurationProperties.NAME;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.entry;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
+import java.net.URL;
+import java.util.Properties;
+
import org.apache.commons.lang.StringUtils;
-import org.apache.geode.test.junit.categories.UnitTest;
import org.junit.Test;
import org.junit.experimental.categories.Category;
-import java.net.MalformedURLException;
-import java.net.URL;
-import java.util.Properties;
-import java.util.concurrent.TimeUnit;
+import org.apache.geode.test.junit.categories.UnitTest;
/**
- * The AbstractLauncherTest class is a test suite of unit tests testing
the contract and
- * functionality of the AbstractLauncher class.
- *
- *
- * @see org.apache.geode.distributed.AbstractLauncher
- * @see org.junit.Assert
- * @see org.junit.Test
+ * Unit tests for {@link AbstractLauncher}.
+ *
* @since GemFire 7.0
*/
@Category(UnitTest.class)
public class AbstractLauncherTest {
- private AbstractLauncher createAbstractLauncher(final String
memberName,
- final String memberId) {
-return new FakeServiceLauncher(memberName, memberId);
- }
-
@Test
- public void shouldBeMockable() throws Exception {
+ public void canBeMocked() throws Exception {
AbstractLauncher mockAbstractLauncher = mock(AbstractLauncher.class);
mockAbstractLauncher.setDebug(true);
verify(mockAbstractLauncher, times(1)).setDebug(true);
}
@Test
- public void testIsSet() {
-final Properties properties = new Properties();
+ public void isSetReturnsFalseIfPropertyDoesNotExist() throws Exception {
+assertThat(AbstractLauncher.isSet(new Properties(), NAME)).isFalse();
+ }
-assertFalse(properties.containsKey(NAME));
-assertFalse(AbstractLauncher.isSet(properties, NAME));
+ @Test
+ public void isSetReturnsFalseIfPropertyHasEmptyValue() throws Exception {
+Properties properties = new Properties();
properties.setProperty(NAME, "");
-assertTrue(properties.containsKey(NAME));
-assertFalse(AbstractLauncher.isSet(properties, NAME));
+assertThat(AbstractLauncher.isSet(properties, NAME)).isFalse();
+ }
+
+ @Test
+ public void isSetReturnsFalseIfPropertyHasBlankValue() throws Exception {
+Properties properties = new Properties();
properties.setProperty(NAME, " ");
-assertTrue(properties.containsKey(NAME));
-assertFalse(AbstractLauncher.isSet(properties, NAME));
+assertThat(AbstractLauncher.isSet(properties, NAME)).isFalse();
+ }
+
+ @Test
+ public void isSetReturnsFalseIfPropertyHasRealValue() throws Exception {
--- End diff --
Shouldn't this test name be 'isSetReturnsTrueIfPropertyHasRealValue'
---
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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132281558
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/AbstractLauncherIntegrationTest.java
---
@@ -47,26 +50,21 @@
@Before
public void setUp() throws Exception {
-this.gemfirePropertiesFile =
-this.temporaryFolder.newFile(DistributionConfig.GEMFIRE_PREFIX +
"properties");
+propertiesFile = temporaryFolder.newFile(GEMFIRE_PREFIX +
"properties");
-this.expectedGemfireProperties = new Properties();
-this.expectedGemfireProperties.setProperty(NAME, "memberOne");
-this.expectedGemfireProperties.setProperty(GROUPS, "groupOne,
groupTwo");
-this.expectedGemfireProperties.store(new
FileWriter(this.gemfirePropertiesFile, false),
-this.testName.getMethodName());
+expectedProperties = new Properties();
+expectedProperties.setProperty(NAME, "memberOne");
+expectedProperties.setProperty(GROUPS, "groupOne, groupTwo");
+expectedProperties.store(new FileWriter(propertiesFile, false),
testName.getMethodName());
-assertThat(this.gemfirePropertiesFile).isNotNull();
-assertThat(this.gemfirePropertiesFile.exists()).isTrue();
-assertThat(this.gemfirePropertiesFile.isFile()).isTrue();
+assertThat(propertiesFile).exists().isFile();
}
@Test
- public void testLoadGemFirePropertiesFromFile() throws Exception {
-final Properties actualGemFireProperties =
-
AbstractLauncher.loadGemFireProperties(this.gemfirePropertiesFile.toURI().toURL());
+ public void loadGemFirePropertiesFromFile() throws Exception {
+Properties loadedProperties =
loadGemFireProperties(propertiesFile.toURI().toURL());
-assertThat(actualGemFireProperties).isNotNull();
-
assertThat(actualGemFireProperties).isEqualTo(this.expectedGemfireProperties);
+assertThat(loadedProperties).isEqualTo(expectedProperties);
}
+
--- End diff --
nit-pick: unnecessary blank line
---
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 #687: GEODE-3258: Refactoring DiskStoreCommands
Github user pdxrunner commented on a diff in the pull request: https://github.com/apache/geode/pull/687#discussion_r131787137 --- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ValidateDiskStoreCommand.java --- @@ -0,0 +1,107 @@ +/* + * 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.apache.geode.management.internal.cli.commands.DiskStoreCommandsUtils.configureLogging; --- End diff -- Remove import. See my previous moment regarding utility method calls. --- 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 #687: GEODE-3258: Refactoring DiskStoreCommands
Github user pdxrunner commented on a diff in the pull request: https://github.com/apache/geode/pull/687#discussion_r131787085 --- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/UpgradeOfflineDiskStoreCommand.java --- @@ -0,0 +1,179 @@ +/* + * 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.apache.geode.management.internal.cli.commands.DiskStoreCommandsUtils.configureLogging; --- End diff -- Remove import. See my previous moment regarding utility method calls. --- 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 #687: GEODE-3258: Refactoring DiskStoreCommands
Github user pdxrunner commented on a diff in the pull request: https://github.com/apache/geode/pull/687#discussion_r131782938 --- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CompactOfflineDiskStoreCommand.java --- @@ -0,0 +1,178 @@ +/* + * 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.apache.geode.management.internal.cli.commands.DiskStoreCommandsUtils.configureLogging; --- End diff -- I would delete this import. Both of the DiskStoreCommandsUtils methods should be treated the same. Either provide static imports for both configureLogging and validatedDirectories, or for neither of them. I would opt for the later so it's obvious from the calls that these are static utility methods. --- 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 #687: GEODE-3258: Refactoring DiskStoreCommands
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/687#discussion_r131777554
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CompactDiskStoreCommand.java
---
@@ -0,0 +1,185 @@
+/*
+ * 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 java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.springframework.shell.core.annotation.CliCommand;
+import org.springframework.shell.core.annotation.CliOption;
+
+import org.apache.geode.cache.persistence.PersistentID;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import
org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.DistributedSystemMXBean;
+import org.apache.geode.management.ManagementService;
+import org.apache.geode.management.cli.CliMetaData;
+import org.apache.geode.management.cli.ConverterHint;
+import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.internal.cli.CliUtil;
+import org.apache.geode.management.internal.cli.LogWrapper;
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.geode.management.internal.cli.result.CompositeResultData;
+import org.apache.geode.management.internal.cli.result.ResultBuilder;
+import org.apache.geode.management.internal.messages.CompactRequest;
+import org.apache.geode.management.internal.security.ResourceOperation;
+import org.apache.geode.security.ResourcePermission;
+
+public class CompactDiskStoreCommand implements GfshCommand {
+ @CliCommand(value = CliStrings.COMPACT_DISK_STORE, help =
CliStrings.COMPACT_DISK_STORE__HELP)
+ @CliMetaData(relatedTopic = {CliStrings.TOPIC_GEODE_DISKSTORE})
+ @ResourceOperation(resource = ResourcePermission.Resource.CLUSTER,
+ operation = ResourcePermission.Operation.MANAGE, target =
ResourcePermission.Target.DISK)
+ 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.GROUP, CliStrings.GROUPS},
+ help = CliStrings.COMPACT_DISK_STORE__GROUP__HELP) String[]
groups) {
+Result result;
+
+try {
+ // disk store exists validation
+ if (!diskStoreExists(diskStoreName)) {
+result = ResultBuilder.createUserErrorResult(
+
CliStrings.format(CliStrings.COMPACT_DISK_STORE__DISKSTORE_0_DOES_NOT_EXIST,
+new Object[] {diskStoreName}));
+ } else {
+InternalDistributedSystem ds =
getCache().getInternalDistributedSystem();
+
+Map<DistributedMember, PersistentID> overallCompactInfo = new
HashMap<>();
+
+Set otherMembers =
ds.getDistributionManager().getOtherNormalDistributionManagerIds();
+Set allMembers = new HashSet<>();
+
+for (Object member : otherMembers) {
+ allMembers.add((InternalDistributedMember) member);
+}
+allMembers.add(ds.getDistributedMember());
+
+String groupInfo = "";
+// if groups are specified, find members in the specified group
+if (groups != null && groups.length > 0) {
+ groupInfo =
CliStrings.format(CliStrings.COMPACT_DISK_STORE__MSG__FOR_GROUP,
+ new Object[] {Arrays.toString(groups) + "."});
+ final Set selectedMembers = new
H
[GitHub] geode issue #687: GEODE-3258: Refactoring DiskStoreCommands
Github user pdxrunner commented on the issue: https://github.com/apache/geode/pull/687 I'll go ahead and review your command refactorings in this PR, keeping in mind that the test will be dealt with under the other JIRA. I agree with not changing those two DUnit tests at this 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 #687: GEODE-3258: Refactoring DiskStoreCommands
Github user pdxrunner commented on the issue: https://github.com/apache/geode/pull/687 With regards to your comments on the test classes and possible duplication: I would consider splitting the existing test classes so that each of the new *Command classes has it's own tests. This may actually mean creating two test classes for each command - a Unit test to cover the functionality that doesn't need the overhead of the DUnit framework, and a DUnit test to cover what can't be accomplished in the Unit test. I would also consider making this refactoring incrementally, with separate pull requests for each command that is extracted from the original DiskStoreCommands class. The incremtnal PR's would then entail creating a Command class and test classes as needed. As a starter, you may want to create the ...Utils class when you first extract a command that needs the methods. --- 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 #685: GEODE-3261: Refactoring GfshHelpCommands
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/685#discussion_r131479491
--- Diff:
geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperIntegrationTest.java
---
@@ -34,7 +36,7 @@
public static void beforeClass() {
helper = new Helper();
// use GfshHelpCommand for testing
-Method[] methods = GfshHelpCommands.class.getMethods();
+Method[] methods = GfshHelpCommand.class.getMethods();
--- End diff --
With the original multiple-command class being split, this test now only
checks one of the two commands. Add a second test for GfshHintCommand. I would
add it to this test class, refactoring the beforeClass functionality into
separate imnplementations in each 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 #651: GEODE-3230: Cleaning up unused (Cli)Strings
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/651#discussion_r130982335
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
---
@@ -633,8 +631,8 @@ public DataCommandResult put(String key, String value,
boolean putIfAbsent, Stri
try {
keyObject = getClassObject(key, keyClass);
} catch (ClassNotFoundException e) {
-return DataCommandResult.createPutResult(key, null, null,
-"ClassNotFoundException " + keyClass, false);
+return DataCommandResult.createPutResult(key, null,
e.getException(),
--- End diff --
I don't understand the context of this change - the message was
"ClassNotFound" but now it's "key not found" with a key class?
---
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 #580: GEODE-2936: Refactoring OrderByComparator
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/580#discussion_r129395826
--- Diff:
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
---
@@ -173,36 +157,58 @@ public void testUnsupportedOrderByForPR() throws
Exception {
@Test
public void testSupportedOrderByForRR() throws Exception {
-
String unsupportedQueries[] =
-{"select distinct p.status from /portfolio1 p order by p.status,
p.ID",
-
-};
+{"select distinct p.status from /portfolio1 p order by p.status,
p.ID"};
Object r[][] = new Object[unsupportedQueries.length][2];
-QueryService qs;
-qs = CacheUtils.getQueryService();
Position.resetCounter();
-// Create Regions
+// Create Regions
Region r1 = CacheUtils.createRegion("portfolio1", Portfolio.class);
for (int i = 0; i < 50; i++) {
r1.put(new Portfolio(i), new Portfolio(i));
}
for (int i = 0; i < unsupportedQueries.length; i++) {
- Query q = null;
-
+ Query q;
CacheUtils.getLogger().info("Executing query: " +
unsupportedQueries[i]);
q = CacheUtils.getQueryService().newQuery(unsupportedQueries[i]);
try {
r[i][0] = q.execute();
-
} catch (QueryInvalidException qe) {
qe.printStackTrace();
fail(qe.toString());
}
}
}
+ /**
+ * Tests three cases that were not originally covered by the original
tests in this class. (To
--- End diff --
This comment probably doesn't need to be a javadoc, and the note about
coverage isn't needed. I'd make this comment say something like, "The following
tests cover edge cases in OrderByComparator".
---
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 #601: GEODE-3117: fix Gateway authentication with legacy ...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/601#discussion_r124376469
--- Diff:
geode-wan/src/test/java/org/apache/geode/internal/cache/wan/misc/GatewayLegacyAuthenticationRegressionTest.java
---
@@ -0,0 +1,429 @@
+/*
+ * 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.cache.wan.misc;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+import static
org.apache.geode.distributed.ConfigurationProperties.DISTRIBUTED_SYSTEM_ID;
+import static
org.apache.geode.distributed.ConfigurationProperties.LOCATORS;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static
org.apache.geode.distributed.ConfigurationProperties.REMOTE_LOCATORS;
+import static
org.apache.geode.distributed.ConfigurationProperties.SECURITY_CLIENT_AUTHENTICATOR;
+import static
org.apache.geode.distributed.ConfigurationProperties.SECURITY_PEER_AUTHENTICATOR;
+import static
org.apache.geode.distributed.ConfigurationProperties.SECURITY_PEER_AUTH_INIT;
+import static
org.apache.geode.distributed.ConfigurationProperties.START_LOCATOR;
+import static org.apache.geode.test.dunit.Host.getHost;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.awaitility.Awaitility.await;
+import static org.awaitility.Awaitility.waitAtMost;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.Serializable;
+import java.security.Principal;
+import java.util.Properties;
+import java.util.UUID;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.LogWriter;
+import org.apache.geode.cache.AttributesFactory;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.DataPolicy;
+import org.apache.geode.cache.DiskStore;
+import org.apache.geode.cache.DiskStoreFactory;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.Scope;
+import org.apache.geode.cache.wan.GatewayReceiver;
+import org.apache.geode.cache.wan.GatewayReceiverFactory;
+import org.apache.geode.cache.wan.GatewaySender;
+import org.apache.geode.cache.wan.GatewaySenderFactory;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.distributed.DistributedSystem;
+import org.apache.geode.internal.AvailablePortHelper;
+import org.apache.geode.internal.cache.wan.InternalGatewaySenderFactory;
+import org.apache.geode.security.AuthInitialize;
+import org.apache.geode.security.AuthenticationFailedException;
+import org.apache.geode.security.Authenticator;
+import org.apache.geode.test.dunit.DistributedTestCase;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.junit.categories.DistributedTest;
+import org.apache.geode.test.junit.categories.SecurityTest;
+import org.apache.geode.test.junit.categories.WanTest;
+import
org.apache.geode.test.junit.rules.serializable.SerializableTemporaryFolder;
+
+/**
+ * Reproduces bug GEODE-3117: "Gateway authentication throws
NullPointerException" and validates the
+ * fix.
+ */
+@Category({DistributedTest.class, SecurityTest.class, WanTest.class})
+public class GatewayLegacyAuthenticationRegressionTest extends
DistributedTestCase {
+
+ private static final String USER_NAME = "security-username";
+ private static final String PASSWORD = "security-password";
+
+ private static final AtomicInteger invokeAuthenticateCount = new
AtomicInteger();
+
+ private static Cache cache;
+ private static GatewaySender sender;
+
+ private VM londonLocatorVM;
+ private VM newYorkLocatorVM;
+ private VM londonServerVM;
+ private VM newYorkServerVM;
+
+ private St
[GitHub] geode pull request #576: Geode 2920, 2921, 2922, 2924
Github user pdxrunner commented on a diff in the pull request: https://github.com/apache/geode/pull/576#discussion_r123578810 --- Diff: geode-core/src/main/java/org/apache/geode/management/CacheServerMXBean.java --- @@ -352,44 +352,44 @@ * * @param clientId ID of the client for which to retrieve information. */ - public ClientHealthStatus showClientStats(String clientId) throws Exception; + ClientHealthStatus showClientStats(String clientId) throws Exception; /** * Returns the number of clients who have existing subscriptions. */ - public int getNumSubscriptions(); + int getNumSubscriptions(); /** * Returns health and statistic information for all clients. Some of the information (CPUs, * NumOfCacheListenerCalls, NumOfGets,NumOfMisses, NumOfPuts,NumOfThreads, ProcessCpuTime) only * available for clients which have set a "StatisticsInterval". */ - public ClientHealthStatus[] showAllClientStats() throws Exception; + ClientHealthStatus[] showAllClientStats() throws Exception; /** * Shows a list of client with their queue statistics. Client queue statistics shown in this * method are the following * - * eventsEnqued,eventsRemoved , eventsConflated ,markerEventsConflated , eventsExpired, + * eventsEnqueued,eventsRemoved , eventsConflated ,markerEventsConflated , eventsExpired, --- End diff -- (Nit) This (and the following javadoc comment) are kinda ugly with the spacing around the commas. While you're fixing the spelling why not also fix the rest. --- 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 #582: GEODE-2601: Fix banner being logged twice
Github user pdxrunner commented on a diff in the pull request: https://github.com/apache/geode/pull/582#discussion_r122332971 --- Diff: geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java --- @@ -469,15 +470,13 @@ private InternalLocator(int port, File logF, File stateF, InternalLogWriter logW LogWriterAppenders.getOrCreateAppender(LogWriterAppenders.Identifier.SECURITY, true, false, this.config, false); --- End diff -- delete extra blank line --- 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 #582: GEODE-2601: Fix banner being logged twice
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/582#discussion_r122332566
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
---
@@ -2167,7 +2191,7 @@ private static void
notifyReconnectListeners(InternalDistributedSystem oldsys,
private void notifyResourceEventListeners(ResourceEvent event, Object
resource) {
for (Iterator iter =
resourceListeners.iterator(); iter.hasNext();) {
--- End diff --
This loop looks like it could be rewritten as a foreach type loop
`
for (ResourceEventListnener listener:resourceListeners) {
...
}
`
---
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 #575: GEODE-3048: Editing tests requiring Gradle test run...
Github user pdxrunner commented on a diff in the pull request: https://github.com/apache/geode/pull/575#discussion_r121690753 --- Diff: geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsIntegrationTest.java --- @@ -39,6 +39,8 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.junit.ClassRule; --- End diff -- Imports should be reordered to the project standard ordering. --- 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 #575: GEODE-3048: Editing tests requiring Gradle test run...
Github user pdxrunner commented on a diff in the pull request: https://github.com/apache/geode/pull/575#discussion_r121690706 --- Diff: geode-assembly/src/test/java/org/apache/geode/rest/internal/web/controllers/RestAPIOnRegionFunctionExecutionDUnitTest.java --- @@ -26,7 +26,9 @@ import java.util.Map; import java.util.Set; +import org.apache.geode.test.dunit.rules.RequiresGeodeHome; --- End diff -- Reorder imports - looks like there are several files that should have the imports reordered. I'd suggest reviewing all of the tests modified and use the IDE to reorder the imports. --- 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 #549: GEODE-2203: gfsh status locator/server - Command no...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/549#discussion_r119419356
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
---
@@ -771,11 +771,19 @@ public Result statusLocator(
CliStrings.STATUS_SERVICE__GFSH_NOT_CONNECTED_ERROR_MESSAGE,
LOCATOR_TERM_NAME));
}
} else {
-final LocatorLauncher locatorLauncher =
-new
LocatorLauncher.Builder().setCommand(LocatorLauncher.Command.STATUS)
-
.setBindAddress(locatorHost).setDebug(isDebugging()).setPid(pid)
-
.setPort(locatorPort).setWorkingDirectory(workingDirectory).build();
-
+final LocatorLauncher locatorLauncher;
+
+if ((locatorHost == null) && (locatorPort == null) &&
(workingDirectory == null)
+&& (pid == null)) {
+ return ResultBuilder.createShellClientErrorResult(
+
String.format(CliStrings.STATUS_LOCATOR__NO_LOCATOR_SPECIFIED_ERROR_MESSAGE,
+ getLocatorId(locatorHost, locatorPort),
+ StringUtils.defaultIfBlank(workingDirectory,
SystemUtils.CURRENT_DIRECTORY)));
--- End diff --
I thought that the simplification of this argument (don't need the
defaultIfBlank call) was in a previous commit, but it seems to have been
reverted in your latest commit.
---
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 #549: GEODE-2203: gfsh status locator/server - Command no...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/549#discussion_r119221757
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
---
@@ -771,11 +771,18 @@ public Result statusLocator(
CliStrings.STATUS_SERVICE__GFSH_NOT_CONNECTED_ERROR_MESSAGE,
LOCATOR_TERM_NAME));
}
} else {
-final LocatorLauncher locatorLauncher =
-new
LocatorLauncher.Builder().setCommand(LocatorLauncher.Command.STATUS)
-
.setBindAddress(locatorHost).setDebug(isDebugging()).setPid(pid)
-
.setPort(locatorPort).setWorkingDirectory(workingDirectory).build();
+final LocatorLauncher locatorLauncher;
+if ((locatorHost == null) && (locatorPort == null) &&
(workingDirectory == null)) {
+ return ResultBuilder.createShellClientErrorResult(
+
String.format(CliStrings.STATUS_LOCATOR__NO_LOCATOR_SPECIFIED_ERROR_MESSAGE,
+ getLocatorId(locatorHost, locatorPort),
+ StringUtils.defaultIfBlank(workingDirectory,
SystemUtils.CURRENT_DIRECTORY)));
--- End diff --
workingDirectory == null in this clause of the if, so this argument can
just be SystemUtils.CURRENT_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.
---
