[GitHub] geode pull request #491: GEODE-2870: Local node function execution failure c...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/491#discussion_r114672970
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/execute/PartitionedRegionFunctionResultSender.java
---
@@ -214,16 +214,15 @@ public void lastResult(Object oneResult) {
private synchronized void lastResult(Object oneResult, ResultCollector
collector,
boolean lastRemoteResult, boolean lastLocalResult, DistributedMember
memberID) {
+
+boolean completedLocal = lastLocalResult |
this.localLastResultRecieved;
--- End diff --
Did you mean to use the bitwise inclusive OR instead of "||"?
---
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 #490: Feature/geode 2852
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/490#discussion_r114673640
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/WaitUntilParallelGatewaySenderFlushedCoordinator.java
---
@@ -46,22 +48,30 @@ public boolean waitUntilFlushed() throws Throwable {
sender.getCancelCriterion().checkCancelInProgress(null);
}
-// Create callables for local buckets
-List callables =
-buildWaitUntilBucketRegionQueueFlushedCallables(pr);
-
-// Submit local callables for execution
ExecutorService service =
this.sender.getDistributionManager().getWaitingThreadPool();
List> callableFutures = new ArrayList<>();
int callableCount = 0;
-if (logger.isDebugEnabled()) {
- logger.debug("WaitUntilParallelGatewaySenderFlushedCoordinator:
Created and being submitted "
- + callables.size() + " callables=" + callables);
-}
-for (Callable callable : callables) {
+long nanosRemaining = unit.toNanos(timeout);
+long endTime = System.nanoTime() + nanosRemaining;
+Set localBucketRegions = getLocalBucketRegions(pr);
+for (BucketRegion br : localBucketRegions) {
+ // timeout exceeded, do not submit more callables, return
localResult false
+ if (System.nanoTime() >= endTime) {
+localResult = false;
+break;
+ }
+ // create and submit callable with updated timeout
+ Callable callable =
createWaitUntilBucketRegionQueueFlushedCallable(
+ (BucketRegionQueue) br, nanosRemaining, TimeUnit.NANOSECONDS);
+ if (logger.isDebugEnabled()) {
+logger.debug(
--- End diff --
You should probably use log4j2's message formatting and parameters:
```java
logger.debug("WaitUntilParallelGatewaySenderFlushedCoordinator: Submitting
callable for bucket {} callable={} nanosRemaining={}", br.getId(), callable,
nanosRemaining);
```
What you have wouldn't cause problems because it's protected by
isDebugEnabled, but that's the log4j2 way if you want to use it.
---
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 #483: GEODE-2853: Change of locator list request interval
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/483#discussion_r114674582
--- Diff:
geode-core/src/test/java/org/apache/geode/cache/client/internal/LocatorTestBase.java
---
@@ -289,6 +289,13 @@ public void run() throws Exception {
}
}
+ protected void startBridgeClient(final String group, final String host,
final int port,
--- End diff --
+1 to Bruce's suggestion
---
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 #475: GEODE-2812: Add API to get list of live locators
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/475#discussion_r114674893
--- Diff:
geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
---
@@ -253,12 +263,15 @@ protected void updateLocatorList(LocatorListResponse
response) {
List locatorResponse = response.getLocators();
List newLocators = new
ArrayList(locatorResponse.size());
+List activeLocators =
--- End diff --
I think "active" is another adjective where you should consider using
"online" instead.
---
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 #475: GEODE-2812: Add API to get list of live locators
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/475#discussion_r114674811
--- Diff: geode-core/src/main/java/org/apache/geode/cache/client/Pool.java
---
@@ -193,13 +193,23 @@
/**
* Returns an unmodifiable list of {@link java.net.InetSocketAddress} of
the locators this pool is
* using. Each locator is either one {@link PoolFactory#addLocator added
explicitly} when the pool
- * was created or were discovered using the explicit locators.
+ * was created.
*
* If a pool has no locators then it can not discover servers or
locators at runtime.
*/
public java.util.List getLocators();
/**
+ * Returns an unmodifiable list of {@link java.net.InetSocketAddress} of
the locators this pool is
+ * using. The returned locator is only the currently living locator
found based on the periodic
+ * locator list request.
+ *
+ * The returned locator list may be slightly old information. If the
locator does not exist, an
+ * empty list is returned.
+ */
+ public java.util.List getLiveLocators();
--- End diff --
We usually use the terms "online" vs "offline" to distinguish between a
member or server that's currently running or not. I would recommend changing
from "live" to "online":
```java
List getOnlineLocators();
```
---
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 #467: GEODE-258: Remove deprecated Cache.getLoggerI18n and getSe...
Github user kirklund commented on the issue:
https://github.com/apache/geode/pull/467
I think you should the log statements to use Log4j2 loggers instead of
changing them to use a different getLogWriter() API. This work was started in
2014 and was never finished.
To convert a class to use Logger, do the following:
```java
import org.apache.logging.log4j.Logger;
import org.apache.geode.internal.logging.LogService;
...
private static final Logger logger = LogService.getLogger();
```
And then change blocks like this:
```java
if ((logger != null) && logger.fineEnabled()) {
logger.fine("RegionSubRegionSnapshot Region entry count =" +
this.entryCount + " for region =" + this.name);
```
To this:
```java
if (logger.isDebugEnabled()) {
logger.debug("RegionSubRegionSnapshot Region entry count ={} for region
={}", this.entryCount, this.name);
```
---
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 #571: GEODE-2601: Fixing banner being logged twice during...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/571#discussion_r121188448
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java
---
@@ -478,9 +478,8 @@ private InternalLocator(int port, File logF, File
stateF, InternalLogWriter logW
if (logWriter == null) {
logWriter = LogWriterFactory.createLogWriterLogger(false, false,
this.config,
!startDistributedSystem);
- if (logger.isDebugEnabled()) {
+ if (logger.isDebugEnabled())
--- End diff --
We've always had {'s in our style guidelines. So most of us are adding them
in for small if-blocks rather than removing 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.
---
[GitHub] geode pull request #571: GEODE-2601: Fixing banner being logged twice during...
Github user kirklund commented on a diff in the pull request: https://github.com/apache/geode/pull/571#discussion_r121188665 --- Diff: geode-core/src/main/java/org/apache/geode/internal/logging/LogWriterFactory.java --- @@ -87,19 +87,19 @@ public static InternalLogWriter createLogWriterLogger(final boolean isLoner, // LOG:CONFIG: logger.info(LogMarker.CONFIG, Banner.getString(null)); } + System.setProperty(InternalLocator.INHIBIT_DM_BANNER, "true"); // Ensure no more banners will --- End diff -- I think we need to find a way to do this without setting INHIBIT_DM_BANNER. If the user closes the Cache and reopens it then this system property will still be true. --- 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 #565: GEODE-3021: Any call after the first to setPdxStrin...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/565#discussion_r121195985
--- Diff:
geode-core/src/main/java/org/apache/geode/cache/query/internal/index/AbstractIndex.java
---
@@ -2002,7 +2002,8 @@ void populateListForEquiJoin(List list, Object
outerEntries, Object innerEntries
*/
synchronized void setPdxStringFlag(Object key) {
// For Null and Undefined keys do not set the isIndexedPdxKeysFlagSet
flag
-if (key == null || key == IndexManager.NULL || key ==
QueryService.UNDEFINED) {
+if (isIndexedPdxKeysFlagSet || key == null || key == IndexManager.NULL
--- End diff --
Looks like you have two flags for the same thing... isIndexedPdxKeys and
isIndexedPdxKeysFlagSet?
---
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 #560: Geode 2818: Adding aliases to any command options t...
Github user kirklund commented on a diff in the pull request: https://github.com/apache/geode/pull/560#discussion_r121197448 --- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java --- @@ -736,7 +736,7 @@ public Result exportData( @CliOption(key = CliStrings.EXPORT_DATA__FILE, unspecifiedDefaultValue = CliMetaData.ANNOTATION_NULL_VALUE, mandatory = true, help = CliStrings.EXPORT_DATA__FILE__HELP) String filePath, - @CliOption(key = CliStrings.EXPORT_DATA__MEMBER, + @CliOption(key = CliStrings.MEMBER, --- End diff -- This makes me wonder if we should file a Jira ticket to make sure all Gfsh commands with MEMBER option are consistently allowing comma-delimited lists of members. --- 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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/557#discussion_r121198226
--- Diff:
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsTest.java
---
@@ -0,0 +1,51 @@
+/*
+ * 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.mockito.Matchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+
+import org.apache.geode.management.internal.cli.GfshParseResult;
+import org.apache.geode.management.internal.cli.shell.Gfsh;
+import org.apache.geode.test.dunit.rules.GfshParserRule;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+import org.springframework.util.ReflectionUtils;
+
+import java.util.Properties;
+
+public class LauncherLifecycleCommandsTest {
--- End diff --
Need a Category on 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 issue #573: GEODE-2622: Fix test failures caused by upgrade to Mockito...
Github user kirklund commented on the issue: https://github.com/apache/geode/pull/573 I just pulled this in to feature/GEODE-2558. I changed the git commit message to: GEODE-2622: Fix GMSMembershipManagerJUnitTest use of Mockito 2.7.11 - Fix 4 test failures in GMSMembershipManagerJUnitTest This closes #573 --- 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 #573: GEODE-2622: Fix test failures caused by upgrade to Mockito...
Github user kirklund commented on the issue: https://github.com/apache/geode/pull/573 Your PR is safely committed to feature/GEODE-2558. I also rebased feature/GEODE-2558 on develop. Normally asfgit (an Apache bot) would close this PR but it won't do that until I merge feature/GEODE-2558 to develop. The new Travis errors are caused by the rebase and can be ignored. You can also safely close this PR now if you want or asfgit will do so eventually when I merge to develop. --- 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 kirklund opened a pull request: https://github.com/apache/geode/pull/601 GEODE-3117: fix Gateway authentication with legacy security * add GatewayLegacyAuthenticationRegressionTest to reproduce bug * fix authentication of Gateway sender/receiver with SECURITY_CLIENT_AUTHENTICATOR Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [x ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [x ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [x ] Is your initial contribution a single, squashed commit? - [x ] Does `gradlew build` run cleanly? - [x ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to dev@geode.apache.org. You can merge this pull request into a Git repository by running: $ git pull https://github.com/kirklund/geode GEM-1523 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/601.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 #601 commit 71a02f6991ce8ce2874c11482b99cba30f5d6d90 Author: Kirk Lund Date: 2017-06-16T23:33:17Z GEODE-3117: fix Gateway authentication with legacy security * add GatewayLegacyAuthenticationRegressionTest to reproduce bug * fix authentication of Gateway sender/receiver with SECURITY_CLIENT_AUTHENTICATOR --- 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 #529: GEODE-2980: All the methods in an interface are implicitly...
Github user kirklund commented on the issue: https://github.com/apache/geode/pull/529 @davinash please rebase on develop to fix the new conflicts and I'll merge to develop. 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 issue #601: GEODE-3117: fix Gateway authentication with legacy securit...
Github user kirklund commented on the issue: https://github.com/apache/geode/pull/601 @pdxrunner thanks for finding the commented out code! I fixed it. --- 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 #623: GEODE-3156: add AcceptanceTest gradle target and ju...
GitHub user kirklund opened a pull request: https://github.com/apache/geode/pull/623 GEODE-3156: add AcceptanceTest gradle target and junit category 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/kirklund/geode acceptanceTests Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/623.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 #623 commit 13a68b2dc6151dd2bfa9d9c89ae6bfeafcc43cd3 Author: Kirk Lund Date: 2017-07-10T21:43:33Z GEODE-3156: add AcceptanceTest gradle target and junit category --- 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 #635: GEODE-2594 Remove tools.jar and --pid options from ...
Github user kirklund commented on a diff in the pull request: https://github.com/apache/geode/pull/635#discussion_r127515789 --- Diff: geode-docs/configuring/running/running_the_locator.html.md.erb --- @@ -35,7 +35,7 @@ You can run the locator standalone or embedded within another Geode process. Run Locator configuration and log files have the following properties: -- When you start a standalone locator using `gfsh`, `gfsh` will automatically load the required JAR files (`$GEMFIRE/lib/locator-dependencies.jar`) into the CLASSPATH of the JVM process. If you start a standalone locator using the `LocatorLauncher` API, you must specify `$GEMFIRE/lib/locator-dependencies.jar` inside the command used to launch the locator process. For more information on CLASSPATH settings in Geode, see [CLASSPATH Settings for Geode Processes](../../getting_started/setup_classpath.html). You can modify the CLASSPATH by specifying the `--classpath` parameter. +- When you start a standalone locator using `gfsh`, `gfsh` will automatically load the required JAR file (`$GEMFIRE/lib/locator-dependencies.jar`) into the CLASSPATH of the JVM process. If you start a standalone locator using the `LocatorLauncher` API, you must specify `$GEMFIRE/lib/locator-dependencies.jar` inside the command used to launch the locator process. For more information on CLASSPATH settings in Geode, see [CLASSPATH Settings for Geode Processes](../../getting_started/setup_classpath.html). You can modify the CLASSPATH by specifying the `--classpath` parameter. --- End diff -- All references to $GEMFIRE/lib/locator-dependencies.jar should change to $GEODE/lib/geode-dependencies.jar --- 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 #635: GEODE-2594 Remove tools.jar and --pid options from ...
Github user kirklund commented on a diff in the pull request: https://github.com/apache/geode/pull/635#discussion_r127515650 --- Diff: geode-docs/configuring/running/running_the_cacheserver.html.md.erb --- @@ -31,7 +31,7 @@ The Geode server is used primarily for hosting long-lived data regions and for r The `gfsh` utility uses a working directory for its configuration files and log files. These are the defaults and configuration options: -- When you start a standalone server using `gfsh`, `gfsh` will automatically load the required JAR files `$GEMFIRE/lib/server-dependencies.jar` and `$JAVA_HOME/lib/tools.jar` into the CLASSPATH of the JVM process. If you start a standalone server using the ServerLauncher API, you must specify `$GEMFIRE/lib/server-dependencies.jar` inside your command to launch the process. For more information on CLASSPATH settings in Geode, see [Setting Up the CLASSPATH](../../getting_started/setup_classpath.html). +- When you start a standalone server using `gfsh`, `gfsh` will automatically load the required JAR file `$GEMFIRE/lib/server-dependencies.jar` into the CLASSPATH of the JVM process. If you start a standalone server using the ServerLauncher API, you must specify `$GEMFIRE/lib/server-dependencies.jar` inside your command to launch the process. For more information on CLASSPATH settings in Geode, see [Setting Up the CLASSPATH](../../getting_started/setup_classpath.html). --- End diff -- All references to $GEMFIRE/lib/server-dependencies.jar should change to $GEODE/lib/geode-dependencies.jar --- 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 kirklund commented on a diff in the pull request: https://github.com/apache/geode/pull/598#discussion_r128354400 --- Diff: geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java --- @@ -14,6 +14,8 @@ */ package org.apache.geode.management.internal.cli.commands; +import java.util.concurrent.TimeUnit; --- End diff -- Is this import used? It looks like just the import was added so it's probably unused. Delete it? --- 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 kirklund commented on a diff in the pull request: https://github.com/apache/geode/pull/598#discussion_r128354422 --- Diff: geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshRule.java --- @@ -26,6 +26,8 @@ import java.util.function.Predicate; import java.util.stream.Collectors; +import javax.mail.Folder; --- End diff -- Is this import used? It looks like just the import was added so it's probably unused. Delete it? --- 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 #623: GEODE-3156: add AcceptanceTest gradle target and junit cat...
Github user kirklund commented on the issue: https://github.com/apache/geode/pull/623 Pulling this PR in now. --- 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 #623: GEODE-3156: add AcceptanceTest gradle target and ju...
Github user kirklund closed the pull request at: https://github.com/apache/geode/pull/623 --- 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 #450: GEODE-2632: create ClientCachePutBench
Github user kirklund commented on the issue: https://github.com/apache/geode/pull/450 I'll wrap up this PR soon. --- 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 #450: GEODE-2632: create ClientCachePutBench
Github user kirklund closed the pull request at: https://github.com/apache/geode/pull/450 --- 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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/580#discussion_r129693128
--- Diff:
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
---
@@ -173,36 +157,54 @@ 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());
}
}
}
+ // The following tests cover edge cases in OrderByComparator.
+ @Test
+ public void testCompareTwoNulls() throws Exception {
+assert (createComparator().compare(null, null) == 0);
+ }
+
+ @Test
+ public void testCompareTwoObjectArrays() throws Exception {
+String[] arrString1 = {"elephant"};
+String[] arrString2 = {"elephants"};
+assert (createComparator().compare(arrString2, arrString1) == 1);
--- End diff --
Same as previous comment. Please use assertThat.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/580#discussion_r129693040
--- Diff:
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
---
@@ -173,36 +157,54 @@ 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());
}
}
}
+ // The following tests cover edge cases in OrderByComparator.
+ @Test
+ public void testCompareTwoNulls() throws Exception {
+assert (createComparator().compare(null, null) == 0);
--- End diff --
"assert" should be used in product code instead of tests. These are the
optional assertions that are enabled when running a Java product with -ea
(enable assertions).
You should use assertThat instead:
```java
assertThat(createComparator().compare(null, null).isEqualTo(0);
```
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129714136
--- Diff:
geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshRule.java
---
@@ -102,35 +107,52 @@ protected ProcessBuilder toProcessBuilder(GfshScript
gfshScript, Path gfshPath,
commandsToExecute.add("-e " + command);
}
-return new ProcessBuilder(commandsToExecute).directory(workingDir);
+ProcessBuilder p = new ProcessBuilder(commandsToExecute);
+p.directory(workingDir);
+
+// TODO PSRhomberg -- Input requested: Is this OS agnostic
+List extendedClasspath = gfshScript.getExtendedClasspath();
+if (!extendedClasspath.isEmpty()) {
+ Map m = p.environment();
+ String classpathKey = "CLASSPATH";
+ String existingJavaArgs = m.get(classpathKey);
+ String specified = String.join(":", extendedClasspath);
--- End diff --
Linux and Mac use ":" as path separator. Use this instead:
org.apache.commons.lang.SystemUtils.PATH_SEPARATOR
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129712729
--- 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 {
--- End diff --
I recommend removing "DUnit" from the name especially since it's not
categorized as DistributedTest.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129712972
--- 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 =
--- End diff --
These get re-instantiated between each test method. Was that the intention
here? Or do you want these to be static final?
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129713345
--- 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
statusCommandWithValidOptionValueShould
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129712861
--- 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();
--- End diff --
Correct keyword ordering (according to the Google style guide we're using)
is "static final"
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129713502
--- 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
statusCommandWithValidOptionValueShould
[GitHub] geode issue #467: GEODE-258: Remove deprecated Cache.getLoggerI18n and getSe...
Github user kirklund commented on the issue:
https://github.com/apache/geode/pull/467
@davinash Sorry to take so long to respond! For the security logger you can
use this:
```java
private static Logger securityLogger =
LogService.getLogger(LogService.SECURITY_LOGGER_NAME);
```
At some point, we should add this to LogService (feel free to add this if
you want to):
```java
public static Logger getSecurityLogger() {
return getLogger(LogService.SECURITY_LOGGER_NAME);
}
```
Then you could use:
```java
private static Logger securityLogger = LogService.getSecurityLogger();
```
---
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 #672: GEODE-3256: Refactoring DataCommands
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/672#discussion_r130944162
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommandsUtils.java
---
@@ -0,0 +1,311 @@
+/*
+ * 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.ArrayList;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Set;
+import java.util.StringTokenizer;
+
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.commons.lang.StringUtils;
+
+import org.apache.geode.LogWriter;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.execute.FunctionService;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.DistributedRegionMXBean;
+import org.apache.geode.management.ManagementService;
+import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.internal.MBeanJMXAdapter;
+import org.apache.geode.management.internal.cli.CliUtil;
+import org.apache.geode.management.internal.cli.LogWrapper;
+import org.apache.geode.management.internal.cli.domain.DataCommandRequest;
+import org.apache.geode.management.internal.cli.domain.DataCommandResult;
+import
org.apache.geode.management.internal.cli.functions.DataCommandFunction;
+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.cli.result.TabularResultData;
+
+public class DataCommandsUtils {
--- End diff --
I would prefer to avoid *Utils classes as well. It's reasonable to
introduce it as a temporary refactoring though just to facilitate breaking
DataCommands up into multiple Command classes. At some point, we should try to
refactor DataCommandUtils to non-static OO classes.
Unfortunately I don't have any examples for changing DataCommandUtils to OO
off the top of my head. That code is going to require some studying to figure
out how to properly organize it.
I guess my recommendation at this point is to pull in these changes as they
are and then @jaredjstewart @YehEmily @jinmeiliao and myself could get together
and mob on restructuring DataCommandUtils. Thoughts?
---
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 #697: GEODE-3407: fix deadlock between JMX and Reconnect
GitHub user kirklund opened a pull request: https://github.com/apache/geode/pull/697 GEODE-3407: fix deadlock between JMX and Reconnect Change InternalClientMembership to not synchronize on CacheFactory by accepting Cache parameter from CacheServerBridge. New regression test confirms bug and this fix. Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [x] Is your initial contribution a single, squashed commit? - [x] Does `gradlew build` run cleanly? - [x] Have you written or updated unit tests to verify your changes? - [x] 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/kirklund/geode feature/GEM-1256 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/697.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 #697 commit ba7a323cbbd68f4de8b0968038fe1a604c861fcb Author: Kirk Lund Date: 2017-08-07T23:39:04Z GEODE-3407: fix deadlock between JMX and Membership Change InternalClientMembership to not synchronize on CacheFactory by accepting Cache parameter from CacheServerBridge. New regression test confirms bug and this fix. --- 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 kirklund opened a pull request: https://github.com/apache/geode/pull/699 GEODE-3413: overhaul launcher and process classes and tests This is primarily an overall of all ServerLauncher and LocatorLauncher tests and org.apache.geode.internal.process tests. The main classes in org.apachage.geode.internal.process package are also cleaned up. In addition, several bugs involving these classes and tests are fixed. Here is the complete list of tickets that are resolved in this overhaul: * GEODE-1229: LocatorLauncherRemoteJUnitTest.testStartOverwritesStalePidFile * GEODE-2791: LocatorLauncherAssemblyIntegrationTest.testLocatorStopsWhenJmxPortIsNonZero fails intermittently with AssertionError * GEODE-1308: CI failure: LocatorLauncherTest.testSetBindAddressToNonLocalHost * GEODE-1309: CI failure: ServerLauncherTest.testSetServerBindAddressToNonLocalHost * GEODE-3193: locator pid file is removed even if there was a problem while shutting down * GEODE-3413: Overhaul launcher tests and process tests * GEODE-3414: Cleanup org.apache.geode.internal.process package Note I moved all useful tests from LocatorLauncherAssemblyIntegrationTest into the other launcher tests in geode-core. You can merge this pull request into a Git repository by running: $ git pull https://github.com/kirklund/geode GEODE-3183-3413-3414 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/699.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 #699 commit 69cdd5cbc7423577b0a5fcde5f8da486437fa387 Author: Kirk Lund Date: 2017-07-10T19:30:09Z GEODE-3413: overhaul launcher and process classes and tests This is primarily an overall of all ServerLauncher and LocatorLauncher tests and org.apache.geode.internal.process tests. The main classes in org.apachage.geode.internal.process package are also cleaned up. In addition, several bugs involving these classes and tests are fixed. Here is the complete list of tickets that are resolved in this overhaul: * GEODE-1229: LocatorLauncherRemoteJUnitTest.testStartOverwritesStalePidFile * GEODE-2791: LocatorLauncherAssemblyIntegrationTest.testLocatorStopsWhenJmxPortIsNonZero fails intermittently with AssertionError * GEODE-1308: CI failure: LocatorLauncherTest.testSetBindAddressToNonLocalHost * GEODE-1309: CI failure: ServerLauncherTest.testSetServerBindAddressToNonLocalHost * GEODE-3193: locator pid file is removed even if there was a problem while shutting down * GEODE-3413: Overhaul launcher tests and process tests * GEODE-3414: Cleanup org.apache.geode.internal.process package Note I moved all useful tests from LocatorLauncherAssemblyIntegrationTest into the other launcher tests in geode-core. --- 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 kirklund commented on a diff in the pull request: https://github.com/apache/geode/pull/699#discussion_r132299177 --- Diff: geode-core/src/main/java/org/apache/geode/config/internal/ClusterConfigurationNotAvailableException.java --- @@ -0,0 +1,29 @@ +/* + * 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.config.internal; --- End diff -- Do we want to use org.apache.geode.internal.config instead? --- 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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132305098
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/process/AbstractProcessStreamReaderIntegrationTest.java
---
@@ -0,0 +1,234 @@
+/*
+ * 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 java.util.concurrent.TimeUnit.MILLISECONDS;
+import static org.apache.commons.lang.SystemUtils.LINE_SEPARATOR;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionFactory;
+import org.junit.After;
+
+import org.apache.geode.internal.util.StopWatch;
+
+/**
+ * Abstract base class for functional integration testing of {@link
ProcessStreamReader}.
+ */
+public abstract class AbstractProcessStreamReaderIntegrationTest {
+
+ /** Timeout to join to a running ProcessStreamReader thread */
+ protected static final int READER_JOIN_TIMEOUT_MILLIS = 20 * 1000;
+
+ /** Sleep timeout for {@link ProcessSleeps} instead of sleeping
Long.MAX_VALUE */
+ private static final int PROCESS_FAILSAFE_TIMEOUT_MILLIS = 10 * 60 *
1000;
+
+ /** Additional time for launched processes to live before terminating */
+ private static final int PROCESS_TIME_TO_LIVE_MILLIS = 3 * 500;
+
+ /** Timeout to wait for a new {@link ProcessStreamReader} to be running
*/
+ private static final int WAIT_FOR_READER_IS_RUNNING_TIMEOUT_MILLIS = 20
* 1000;
+
+ protected Process process;
+ protected ProcessStreamReader stderr;
+ protected ProcessStreamReader stdout;
+
+ @After
+ public void afterProcessStreamReaderTestCase() throws Exception {
+if (stderr != null) {
+ stderr.stop();
+}
+if (stdout != null) {
+ stdout.stop();
+}
+if (process != null) {
+ try {
+process.getErrorStream().close();
+process.getInputStream().close();
+process.getOutputStream().close();
+ } finally {
+// this is async and can require more than 10 seconds on slower
machines
+process.destroy();
+ }
+}
+ }
+
+ protected ConditionFactory await() {
+return
Awaitility.await().atMost(WAIT_FOR_READER_IS_RUNNING_TIMEOUT_MILLIS,
MILLISECONDS);
+ }
+
+ protected static boolean isAlive(final Process process) {
--- End diff --
All callers to this method should now be changed to use the new API instead
of this old hack:
```java
process.isAlive()
```
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132307026
--- 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 --
Nice catch!
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132308040
--- 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, "");
--- End diff --
Done! I made this change in all launcher and process main classes and test
classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at 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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132308711
--- 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 --
Removed from all classes in change-set.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132312206
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/process/FileProcessControllerIntegrationTest.java
---
@@ -0,0 +1,243 @@
+/*
+ * 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 com.googlecode.catchexception.CatchException.caughtException;
+import static com.googlecode.catchexception.CatchException.verifyException;
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.MINUTES;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.File;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionFactory;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.ErrorCollector;
+import org.junit.rules.TemporaryFolder;
+import org.junit.rules.TestName;
+
+import org.apache.geode.distributed.AbstractLauncher.Status;
+import org.apache.geode.distributed.LocatorLauncher;
+import org.apache.geode.distributed.LocatorLauncher.Builder;
+import org.apache.geode.distributed.LocatorLauncher.LocatorState;
+import org.apache.geode.internal.process.io.EmptyFileWriter;
+import org.apache.geode.internal.process.io.IntegerFileWriter;
+import org.apache.geode.internal.process.io.StringFileWriter;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+
+/**
+ * Integration tests for {@link FileProcessController}.
+ */
+@Category(IntegrationTest.class)
+public class FileProcessControllerIntegrationTest {
+
+ private static final String STATUS_JSON = generateStatusJson();
+
+ private final AtomicReference statusRef = new
AtomicReference<>();
+
+ private File pidFile;
+ private File statusFile;
+ private File statusRequestFile;
+ private File stopRequestFile;
+ private int pid;
+ private FileControllerParameters params;
+ private ExecutorService executor;
+
+ @Rule
+ public ErrorCollector errorCollector = new ErrorCollector();
+
+ @Rule
+ public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+ @Rule
+ public TestName testName = new TestName();
+
+ @Before
+ public void setUp() throws Exception {
+ProcessType processType = ProcessType.LOCATOR;
+File directory = temporaryFolder.getRoot();
+pidFile = new File(directory, processType.getPidFileName());
+statusFile = new File(directory, processType.getStatusFileName());
+statusRequestFile = new File(directory,
processType.getStatusRequestFileName());
+stopRequestFile = new File(directory,
processType.getStopRequestFileName());
+pid = ProcessUtils.identifyPid();
+
+params = mock(FileControllerParameters.class);
+when(params.getPidFile()).thenReturn(pidFile);
+when(params.getProcessId()).thenReturn(pid);
+when(params.getProcessType()).thenReturn(processType);
+when(params.getDirectory()).thenReturn(temporaryFolder.getRoot());
+
+executor = Executors.newSingleThreadExecutor();
+ }
+
+ @After
+ public void tearDown() throws Exception {
+assertThat(executor.shutdownNow()).isEmpty();
+ }
+
+ @Test
+ public void statusShouldAwaitTimeoutWhileFileIsEmpty() throws Exception {
+// given: FileProcessController with empty pidFile
+FilePr
[GitHub] geode pull request #699: GEODE-3413: overhaul launcher and process classes a...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132321846
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTestCase.java
---
@@ -0,0 +1,270 @@
+/*
+ * 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.LOG_LEVEL;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static
org.apache.geode.distributed.internal.DistributionConfig.GEMFIRE_PREFIX;
+import static
org.apache.geode.internal.cache.AbstractCacheServer.TEST_OVERRIDE_DEFAULT_PORT_PROPERTY;
+import static
org.apache.geode.internal.process.ProcessUtils.isProcessAlive;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.BindException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.junit.After;
+import org.junit.Before;
+
+import org.apache.geode.distributed.AbstractLauncher.Status;
+import org.apache.geode.internal.process.ProcessStreamReader;
+import org.apache.geode.internal.process.ProcessStreamReader.InputListener;
+
+/**
+ * Abstract base class for integration tests of {@link ServerLauncher} as
an application main in a
+ * forked JVM.
+ *
+ * @since GemFire 8.0
+ */
+public abstract class ServerLauncherRemoteIntegrationTestCase
+extends ServerLauncherIntegrationTestCase implements UsesServerCommand
{
+
+ private final AtomicBoolean threwBindException = new AtomicBoolean();
+
+ protected volatile Process process;
+ protected volatile ProcessStreamReader processOutReader;
+ protected volatile ProcessStreamReader processErrReader;
+
+ private ServerCommand serverCommand;
+
+ @Before
+ public void setUp() throws Exception {
+serverCommand = new ServerCommand(this);
+ }
+
+ @After
+ public void tearDownAbstractServerLauncherRemoteIntegrationTestCase()
throws Exception {
+if (this.process != null) {
+ this.process.destroy();
+ this.process = null;
+}
+if (this.processOutReader != null &&
this.processOutReader.isRunning()) {
+ this.processOutReader.stop();
+}
+if (this.processErrReader != null &&
this.processErrReader.isRunning()) {
+ this.processErrReader.stop();
+}
+ }
+
+ @Override
+ public List getJvmArguments() {
+List jvmArguments = new ArrayList<>();
+jvmArguments.add("-D" + GEMFIRE_PREFIX + LOG_LEVEL + "=config");
+jvmArguments.add("-D" + GEMFIRE_PREFIX + MCAST_PORT + "=0");
+jvmArguments
+.add("-D" + TEST_OVERRIDE_DEFAULT_PORT_PROPERTY + "=" +
String.valueOf(defaultServerPort));
+return jvmArguments;
+ }
+
+ @Override
+ public String getName() {
+return getUniqueName();
+ }
+
+ @Override
+ public boolean getDisableDefaultServer() {
+return false;
+ }
+
+ protected void assertThatServerThrewBindException() {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected void assertThatServerThrew(final Class
throwableClass) {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected ServerLauncher startServer(final ServerCommand command) {
+return awaitStart(command);
+ }
+
+ protected ServerLauncher awaitStart(final ServerCommand command,
+ final ProcessStreamReader.InputListener outListener,
+ final ProcessStreamReader.InputListener errListener) throws
Exception {
+executeCommandWithReaders(command.
[GitHub] geode pull request #699: GEODE-3413: overhaul launcher and process classes a...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132321803
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTestCase.java
---
@@ -0,0 +1,270 @@
+/*
+ * 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.LOG_LEVEL;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static
org.apache.geode.distributed.internal.DistributionConfig.GEMFIRE_PREFIX;
+import static
org.apache.geode.internal.cache.AbstractCacheServer.TEST_OVERRIDE_DEFAULT_PORT_PROPERTY;
+import static
org.apache.geode.internal.process.ProcessUtils.isProcessAlive;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.BindException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.junit.After;
+import org.junit.Before;
+
+import org.apache.geode.distributed.AbstractLauncher.Status;
+import org.apache.geode.internal.process.ProcessStreamReader;
+import org.apache.geode.internal.process.ProcessStreamReader.InputListener;
+
+/**
+ * Abstract base class for integration tests of {@link ServerLauncher} as
an application main in a
+ * forked JVM.
+ *
+ * @since GemFire 8.0
+ */
+public abstract class ServerLauncherRemoteIntegrationTestCase
+extends ServerLauncherIntegrationTestCase implements UsesServerCommand
{
+
+ private final AtomicBoolean threwBindException = new AtomicBoolean();
+
+ protected volatile Process process;
+ protected volatile ProcessStreamReader processOutReader;
+ protected volatile ProcessStreamReader processErrReader;
+
+ private ServerCommand serverCommand;
+
+ @Before
+ public void setUp() throws Exception {
+serverCommand = new ServerCommand(this);
+ }
+
+ @After
+ public void tearDownAbstractServerLauncherRemoteIntegrationTestCase()
throws Exception {
+if (this.process != null) {
+ this.process.destroy();
+ this.process = null;
+}
+if (this.processOutReader != null &&
this.processOutReader.isRunning()) {
+ this.processOutReader.stop();
+}
+if (this.processErrReader != null &&
this.processErrReader.isRunning()) {
+ this.processErrReader.stop();
+}
+ }
+
+ @Override
+ public List getJvmArguments() {
+List jvmArguments = new ArrayList<>();
+jvmArguments.add("-D" + GEMFIRE_PREFIX + LOG_LEVEL + "=config");
+jvmArguments.add("-D" + GEMFIRE_PREFIX + MCAST_PORT + "=0");
+jvmArguments
+.add("-D" + TEST_OVERRIDE_DEFAULT_PORT_PROPERTY + "=" +
String.valueOf(defaultServerPort));
+return jvmArguments;
+ }
+
+ @Override
+ public String getName() {
+return getUniqueName();
+ }
+
+ @Override
+ public boolean getDisableDefaultServer() {
+return false;
+ }
+
+ protected void assertThatServerThrewBindException() {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected void assertThatServerThrew(final Class
throwableClass) {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected ServerLauncher startServer(final ServerCommand command) {
+return awaitStart(command);
+ }
+
+ protected ServerLauncher awaitStart(final ServerCommand command,
+ final ProcessStreamReader.InputListener outListener,
+ final ProcessStreamReader.InputListener errListener) throws
Exception {
+executeCommandWithReaders(command.
[GitHub] geode pull request #699: GEODE-3413: overhaul launcher and process classes a...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132321779
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTestCase.java
---
@@ -0,0 +1,270 @@
+/*
+ * 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.LOG_LEVEL;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static
org.apache.geode.distributed.internal.DistributionConfig.GEMFIRE_PREFIX;
+import static
org.apache.geode.internal.cache.AbstractCacheServer.TEST_OVERRIDE_DEFAULT_PORT_PROPERTY;
+import static
org.apache.geode.internal.process.ProcessUtils.isProcessAlive;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.BindException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.junit.After;
+import org.junit.Before;
+
+import org.apache.geode.distributed.AbstractLauncher.Status;
+import org.apache.geode.internal.process.ProcessStreamReader;
+import org.apache.geode.internal.process.ProcessStreamReader.InputListener;
+
+/**
+ * Abstract base class for integration tests of {@link ServerLauncher} as
an application main in a
+ * forked JVM.
+ *
+ * @since GemFire 8.0
+ */
+public abstract class ServerLauncherRemoteIntegrationTestCase
+extends ServerLauncherIntegrationTestCase implements UsesServerCommand
{
+
+ private final AtomicBoolean threwBindException = new AtomicBoolean();
+
+ protected volatile Process process;
+ protected volatile ProcessStreamReader processOutReader;
+ protected volatile ProcessStreamReader processErrReader;
+
+ private ServerCommand serverCommand;
+
+ @Before
+ public void setUp() throws Exception {
+serverCommand = new ServerCommand(this);
+ }
+
+ @After
+ public void tearDownAbstractServerLauncherRemoteIntegrationTestCase()
throws Exception {
+if (this.process != null) {
+ this.process.destroy();
+ this.process = null;
+}
+if (this.processOutReader != null &&
this.processOutReader.isRunning()) {
+ this.processOutReader.stop();
+}
+if (this.processErrReader != null &&
this.processErrReader.isRunning()) {
+ this.processErrReader.stop();
+}
+ }
+
+ @Override
+ public List getJvmArguments() {
+List jvmArguments = new ArrayList<>();
+jvmArguments.add("-D" + GEMFIRE_PREFIX + LOG_LEVEL + "=config");
+jvmArguments.add("-D" + GEMFIRE_PREFIX + MCAST_PORT + "=0");
+jvmArguments
+.add("-D" + TEST_OVERRIDE_DEFAULT_PORT_PROPERTY + "=" +
String.valueOf(defaultServerPort));
+return jvmArguments;
+ }
+
+ @Override
+ public String getName() {
+return getUniqueName();
+ }
+
+ @Override
+ public boolean getDisableDefaultServer() {
+return false;
+ }
+
+ protected void assertThatServerThrewBindException() {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected void assertThatServerThrew(final Class
throwableClass) {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected ServerLauncher startServer(final ServerCommand command) {
+return awaitStart(command);
+ }
+
+ protected ServerLauncher awaitStart(final ServerCommand command,
+ final ProcessStreamReader.InputListener outListener,
+ final ProcessStreamReader.InputListener errListener) throws
Exception {
+executeCommandWithReaders(command.
[GitHub] geode pull request #699: GEODE-3413: overhaul launcher and process classes a...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132321751
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTestCase.java
---
@@ -0,0 +1,270 @@
+/*
+ * 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.LOG_LEVEL;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static
org.apache.geode.distributed.internal.DistributionConfig.GEMFIRE_PREFIX;
+import static
org.apache.geode.internal.cache.AbstractCacheServer.TEST_OVERRIDE_DEFAULT_PORT_PROPERTY;
+import static
org.apache.geode.internal.process.ProcessUtils.isProcessAlive;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.BindException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.junit.After;
+import org.junit.Before;
+
+import org.apache.geode.distributed.AbstractLauncher.Status;
+import org.apache.geode.internal.process.ProcessStreamReader;
+import org.apache.geode.internal.process.ProcessStreamReader.InputListener;
+
+/**
+ * Abstract base class for integration tests of {@link ServerLauncher} as
an application main in a
+ * forked JVM.
+ *
+ * @since GemFire 8.0
+ */
+public abstract class ServerLauncherRemoteIntegrationTestCase
+extends ServerLauncherIntegrationTestCase implements UsesServerCommand
{
+
+ private final AtomicBoolean threwBindException = new AtomicBoolean();
+
+ protected volatile Process process;
+ protected volatile ProcessStreamReader processOutReader;
+ protected volatile ProcessStreamReader processErrReader;
+
+ private ServerCommand serverCommand;
+
+ @Before
+ public void setUp() throws Exception {
+serverCommand = new ServerCommand(this);
+ }
+
+ @After
+ public void tearDownAbstractServerLauncherRemoteIntegrationTestCase()
throws Exception {
+if (this.process != null) {
+ this.process.destroy();
+ this.process = null;
+}
+if (this.processOutReader != null &&
this.processOutReader.isRunning()) {
+ this.processOutReader.stop();
+}
+if (this.processErrReader != null &&
this.processErrReader.isRunning()) {
+ this.processErrReader.stop();
+}
+ }
+
+ @Override
+ public List getJvmArguments() {
+List jvmArguments = new ArrayList<>();
+jvmArguments.add("-D" + GEMFIRE_PREFIX + LOG_LEVEL + "=config");
+jvmArguments.add("-D" + GEMFIRE_PREFIX + MCAST_PORT + "=0");
+jvmArguments
+.add("-D" + TEST_OVERRIDE_DEFAULT_PORT_PROPERTY + "=" +
String.valueOf(defaultServerPort));
+return jvmArguments;
+ }
+
+ @Override
+ public String getName() {
+return getUniqueName();
+ }
+
+ @Override
+ public boolean getDisableDefaultServer() {
+return false;
+ }
+
+ protected void assertThatServerThrewBindException() {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected void assertThatServerThrew(final Class
throwableClass) {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected ServerLauncher startServer(final ServerCommand command) {
+return awaitStart(command);
+ }
+
+ protected ServerLauncher awaitStart(final ServerCommand command,
+ final ProcessStreamReader.InputListener outListener,
+ final ProcessStreamReader.InputListener errListener) throws
Exception {
+executeCommandWithReaders(command.
[GitHub] geode pull request #699: GEODE-3413: overhaul launcher and process classes a...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132321738
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTestCase.java
---
@@ -0,0 +1,270 @@
+/*
+ * 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.LOG_LEVEL;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static
org.apache.geode.distributed.internal.DistributionConfig.GEMFIRE_PREFIX;
+import static
org.apache.geode.internal.cache.AbstractCacheServer.TEST_OVERRIDE_DEFAULT_PORT_PROPERTY;
+import static
org.apache.geode.internal.process.ProcessUtils.isProcessAlive;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.BindException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.junit.After;
+import org.junit.Before;
+
+import org.apache.geode.distributed.AbstractLauncher.Status;
+import org.apache.geode.internal.process.ProcessStreamReader;
+import org.apache.geode.internal.process.ProcessStreamReader.InputListener;
+
+/**
+ * Abstract base class for integration tests of {@link ServerLauncher} as
an application main in a
+ * forked JVM.
+ *
+ * @since GemFire 8.0
+ */
+public abstract class ServerLauncherRemoteIntegrationTestCase
+extends ServerLauncherIntegrationTestCase implements UsesServerCommand
{
+
+ private final AtomicBoolean threwBindException = new AtomicBoolean();
+
+ protected volatile Process process;
+ protected volatile ProcessStreamReader processOutReader;
+ protected volatile ProcessStreamReader processErrReader;
+
+ private ServerCommand serverCommand;
+
+ @Before
+ public void setUp() throws Exception {
+serverCommand = new ServerCommand(this);
+ }
+
+ @After
+ public void tearDownAbstractServerLauncherRemoteIntegrationTestCase()
throws Exception {
+if (this.process != null) {
+ this.process.destroy();
+ this.process = null;
+}
+if (this.processOutReader != null &&
this.processOutReader.isRunning()) {
+ this.processOutReader.stop();
+}
+if (this.processErrReader != null &&
this.processErrReader.isRunning()) {
+ this.processErrReader.stop();
+}
+ }
+
+ @Override
+ public List getJvmArguments() {
+List jvmArguments = new ArrayList<>();
+jvmArguments.add("-D" + GEMFIRE_PREFIX + LOG_LEVEL + "=config");
+jvmArguments.add("-D" + GEMFIRE_PREFIX + MCAST_PORT + "=0");
+jvmArguments
+.add("-D" + TEST_OVERRIDE_DEFAULT_PORT_PROPERTY + "=" +
String.valueOf(defaultServerPort));
+return jvmArguments;
+ }
+
+ @Override
+ public String getName() {
+return getUniqueName();
+ }
+
+ @Override
+ public boolean getDisableDefaultServer() {
+return false;
+ }
+
+ protected void assertThatServerThrewBindException() {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected void assertThatServerThrew(final Class
throwableClass) {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected ServerLauncher startServer(final ServerCommand command) {
+return awaitStart(command);
+ }
+
+ protected ServerLauncher awaitStart(final ServerCommand command,
+ final ProcessStreamReader.InputListener outListener,
+ final ProcessStreamReader.InputListener errListener) throws
Exception {
+executeCommandWithReaders(command.
[GitHub] geode pull request #699: GEODE-3413: overhaul launcher and process classes a...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132322055
--- 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 {
+Properties properties = new Properties();
properties.setProperty(NAME, "memberOne");
-assertTrue(AbstractLauncher.isSet(properties, NAME));
-assertFalse(AbstractLauncher.isSet(properties, "NaMe"));
+assertThat(AbstractLauncher.isSet(properties, NAME)).isTrue();
}
@Test
- public void testLoadGemFirePropertiesWithNullURL() {
-final Properties properties =
AbstractLauncher.loadGemFireProperties(null);
-assertNotNull(properties);
-assertTrue(properties.isEmpty());
+ public void isSetKeyIsCaseSensitive() throws Exception {
+Properties properties = new Properties();
+
+properties.setProperty(NAME, "memberOne");
+
+assertThat(AbstractLauncher.isSet(properties, "NaMe")).isFalse();
}
@Test
- publ
[GitHub] geode pull request #699: GEODE-3413: overhaul launcher and process classes a...
Github user kirklund commented on a diff in the pull request: https://github.com/apache/geode/pull/699#discussion_r132323674 --- 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 -- I'm not sure how to test this. For example, my first idea is to invoke it twice and make sure they're not the same but I don't think that's a safe assertion. It's also probably not safe to assert that they're not sequential. Hmm... --- 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 kirklund commented on a diff in the pull request: https://github.com/apache/geode/pull/699#discussion_r132324550 --- 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 -- Actually, just realized AvailablePid returns unused pids. Nothing in the javadoc or API says anything about it being "random" -- that's just how it's implemented. So the only really important details is that the pid is unused. --- 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 kirklund commented on a diff in the pull request: https://github.com/apache/geode/pull/699#discussion_r132324608 --- 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 -- Oops there it is in the javadocs of 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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132326281
--- 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 --
```java
@Test(timeout = DEFAULT_TIMEOUT_MILLIS)
public void findAvailablePidShouldReturnRandomPid() throws Exception {
Random random = spy(new Random());
availablePid = new AvailablePid(random, DEFAULT_TIMEOUT_MILLIS);
availablePid.findAvailablePid();
verify(random, atLeastOnce()).nextInt(anyInt());
}
```
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132532641
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/process/FileProcessControllerIntegrationTest.java
---
@@ -0,0 +1,243 @@
+/*
+ * 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 com.googlecode.catchexception.CatchException.caughtException;
+import static com.googlecode.catchexception.CatchException.verifyException;
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.MINUTES;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.File;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionFactory;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.ErrorCollector;
+import org.junit.rules.TemporaryFolder;
+import org.junit.rules.TestName;
+
+import org.apache.geode.distributed.AbstractLauncher.Status;
+import org.apache.geode.distributed.LocatorLauncher;
+import org.apache.geode.distributed.LocatorLauncher.Builder;
+import org.apache.geode.distributed.LocatorLauncher.LocatorState;
+import org.apache.geode.internal.process.io.EmptyFileWriter;
+import org.apache.geode.internal.process.io.IntegerFileWriter;
+import org.apache.geode.internal.process.io.StringFileWriter;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+
+/**
+ * Integration tests for {@link FileProcessController}.
+ */
+@Category(IntegrationTest.class)
+public class FileProcessControllerIntegrationTest {
+
+ private static final String STATUS_JSON = generateStatusJson();
+
+ private final AtomicReference statusRef = new
AtomicReference<>();
+
+ private File pidFile;
+ private File statusFile;
+ private File statusRequestFile;
+ private File stopRequestFile;
+ private int pid;
+ private FileControllerParameters params;
+ private ExecutorService executor;
+
+ @Rule
+ public ErrorCollector errorCollector = new ErrorCollector();
+
+ @Rule
+ public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+ @Rule
+ public TestName testName = new TestName();
+
+ @Before
+ public void setUp() throws Exception {
+ProcessType processType = ProcessType.LOCATOR;
+File directory = temporaryFolder.getRoot();
+pidFile = new File(directory, processType.getPidFileName());
+statusFile = new File(directory, processType.getStatusFileName());
+statusRequestFile = new File(directory,
processType.getStatusRequestFileName());
+stopRequestFile = new File(directory,
processType.getStopRequestFileName());
+pid = ProcessUtils.identifyPid();
+
+params = mock(FileControllerParameters.class);
+when(params.getPidFile()).thenReturn(pidFile);
+when(params.getProcessId()).thenReturn(pid);
+when(params.getProcessType()).thenReturn(processType);
+when(params.getDirectory()).thenReturn(temporaryFolder.getRoot());
+
+executor = Executors.newSingleThreadExecutor();
+ }
+
+ @After
+ public void tearDown() throws Exception {
+assertThat(executor.shutdownNow()).isEmpty();
+ }
+
+ @Test
+ public void statusShouldAwaitTimeoutWhileFileIsEmpty() throws Exception {
+// given: FileProcessController with empty pidFile
+FilePr
[GitHub] geode pull request #699: GEODE-3413: overhaul launcher and process classes a...
Github user kirklund commented on a diff in the pull request: https://github.com/apache/geode/pull/699#discussion_r132532612 --- Diff: geode-core/src/main/java/org/apache/geode/config/internal/ClusterConfigurationNotAvailableException.java --- @@ -0,0 +1,29 @@ +/* + * 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.config.internal; --- End diff -- Changed. --- 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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132532669
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/process/AbstractProcessStreamReaderIntegrationTest.java
---
@@ -0,0 +1,234 @@
+/*
+ * 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 java.util.concurrent.TimeUnit.MILLISECONDS;
+import static org.apache.commons.lang.SystemUtils.LINE_SEPARATOR;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionFactory;
+import org.junit.After;
+
+import org.apache.geode.internal.util.StopWatch;
+
+/**
+ * Abstract base class for functional integration testing of {@link
ProcessStreamReader}.
+ */
+public abstract class AbstractProcessStreamReaderIntegrationTest {
+
+ /** Timeout to join to a running ProcessStreamReader thread */
+ protected static final int READER_JOIN_TIMEOUT_MILLIS = 20 * 1000;
+
+ /** Sleep timeout for {@link ProcessSleeps} instead of sleeping
Long.MAX_VALUE */
+ private static final int PROCESS_FAILSAFE_TIMEOUT_MILLIS = 10 * 60 *
1000;
+
+ /** Additional time for launched processes to live before terminating */
+ private static final int PROCESS_TIME_TO_LIVE_MILLIS = 3 * 500;
+
+ /** Timeout to wait for a new {@link ProcessStreamReader} to be running
*/
+ private static final int WAIT_FOR_READER_IS_RUNNING_TIMEOUT_MILLIS = 20
* 1000;
+
+ protected Process process;
+ protected ProcessStreamReader stderr;
+ protected ProcessStreamReader stdout;
+
+ @After
+ public void afterProcessStreamReaderTestCase() throws Exception {
+if (stderr != null) {
+ stderr.stop();
+}
+if (stdout != null) {
+ stdout.stop();
+}
+if (process != null) {
+ try {
+process.getErrorStream().close();
+process.getInputStream().close();
+process.getOutputStream().close();
+ } finally {
+// this is async and can require more than 10 seconds on slower
machines
+process.destroy();
+ }
+}
+ }
+
+ protected ConditionFactory await() {
+return
Awaitility.await().atMost(WAIT_FOR_READER_IS_RUNNING_TIMEOUT_MILLIS,
MILLISECONDS);
+ }
+
+ protected static boolean isAlive(final Process process) {
--- End diff --
Done.
---
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 kirklund commented on a diff in the pull request: https://github.com/apache/geode/pull/699#discussion_r132533643 --- Diff: geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java --- @@ -31,7 +31,7 @@ import org.apache.geode.internal.admin.remote.DistributionLocatorId; import org.apache.geode.internal.i18n.LocalizedStrings; import org.apache.geode.internal.logging.LogService; -import org.apache.geode.internal.process.ClusterConfigurationNotAvailableException; +import org.apache.geode.config.internal.ClusterConfigurationNotAvailableException; --- End diff -- Done. --- 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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132535229
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/AbstractLauncher.java ---
@@ -157,7 +162,7 @@ protected static Properties loadGemFireProperties(final
URL url) {
if (url != null) {
try {
properties.load(new FileReader(new File(url.toURI(;
- } catch (Exception e) {
+ } catch (Exception ignored) {
--- End diff --
Done.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132541592
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/AbstractLauncher.java ---
@@ -502,7 +507,7 @@ protected static Integer identifyPid() {
}
}
-// TODO refactor the logic in this method into a DateTimeFormatUtils
class
+// consider refactoring the logic in this method into a
DateTimeFormatUtils class
--- End diff --
Deleted.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132541795
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/process/MBeanProcessController.java
---
@@ -228,9 +230,9 @@ private void disconnect() {
* @throws PidUnavailableException if parsing the pid from the
RuntimeMXBean name fails
*/
boolean checkPidMatches() throws IllegalStateException, IOException,
PidUnavailableException {
--- End diff --
Deleted.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132541957
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java ---
@@ -1848,8 +1884,7 @@ public LocatorLauncher build() {
private final String name;
Command(final String name, final String... options) {
- assert !StringUtils
- .isBlank(name) : "The name of the locator launcher command must
be specified!";
+ assert !isBlank(name) : "The name of the locator launcher command
must be specified!";
--- End diff --
Changed.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132542008
--- 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 --
I added description.
---
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 kirklund commented on a diff in the pull request: https://github.com/apache/geode/pull/699#discussion_r132542016 --- 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 -- I added description. --- 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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132542063
--- 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 --
All unused fields deleted.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132542106
--- 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 --
All unused fields deleted.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132542157
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTestCase.java
---
@@ -0,0 +1,270 @@
+/*
+ * 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.LOG_LEVEL;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static
org.apache.geode.distributed.internal.DistributionConfig.GEMFIRE_PREFIX;
+import static
org.apache.geode.internal.cache.AbstractCacheServer.TEST_OVERRIDE_DEFAULT_PORT_PROPERTY;
+import static
org.apache.geode.internal.process.ProcessUtils.isProcessAlive;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.BindException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.junit.After;
+import org.junit.Before;
+
+import org.apache.geode.distributed.AbstractLauncher.Status;
+import org.apache.geode.internal.process.ProcessStreamReader;
+import org.apache.geode.internal.process.ProcessStreamReader.InputListener;
+
+/**
+ * Abstract base class for integration tests of {@link ServerLauncher} as
an application main in a
+ * forked JVM.
+ *
+ * @since GemFire 8.0
+ */
+public abstract class ServerLauncherRemoteIntegrationTestCase
+extends ServerLauncherIntegrationTestCase implements UsesServerCommand
{
+
+ private final AtomicBoolean threwBindException = new AtomicBoolean();
+
+ protected volatile Process process;
+ protected volatile ProcessStreamReader processOutReader;
+ protected volatile ProcessStreamReader processErrReader;
+
+ private ServerCommand serverCommand;
+
+ @Before
+ public void setUp() throws Exception {
+serverCommand = new ServerCommand(this);
+ }
+
+ @After
+ public void tearDownAbstractServerLauncherRemoteIntegrationTestCase()
throws Exception {
+if (this.process != null) {
+ this.process.destroy();
+ this.process = null;
+}
+if (this.processOutReader != null &&
this.processOutReader.isRunning()) {
+ this.processOutReader.stop();
+}
+if (this.processErrReader != null &&
this.processErrReader.isRunning()) {
+ this.processErrReader.stop();
+}
+ }
+
+ @Override
+ public List getJvmArguments() {
+List jvmArguments = new ArrayList<>();
+jvmArguments.add("-D" + GEMFIRE_PREFIX + LOG_LEVEL + "=config");
+jvmArguments.add("-D" + GEMFIRE_PREFIX + MCAST_PORT + "=0");
+jvmArguments
+.add("-D" + TEST_OVERRIDE_DEFAULT_PORT_PROPERTY + "=" +
String.valueOf(defaultServerPort));
+return jvmArguments;
+ }
+
+ @Override
+ public String getName() {
+return getUniqueName();
+ }
+
+ @Override
+ public boolean getDisableDefaultServer() {
+return false;
+ }
+
+ protected void assertThatServerThrewBindException() {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected void assertThatServerThrew(final Class
throwableClass) {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected ServerLauncher startServer(final ServerCommand command) {
+return awaitStart(command);
+ }
+
+ protected ServerLauncher awaitStart(final ServerCommand command,
+ final ProcessStreamReader.InputListener outListener,
+ final ProcessStreamReader.InputListener errListener) throws
Exception {
+executeCommandWithReaders(command.
[GitHub] geode pull request #699: GEODE-3413: overhaul launcher and process classes a...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132542169
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTestCase.java
---
@@ -0,0 +1,270 @@
+/*
+ * 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.LOG_LEVEL;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static
org.apache.geode.distributed.internal.DistributionConfig.GEMFIRE_PREFIX;
+import static
org.apache.geode.internal.cache.AbstractCacheServer.TEST_OVERRIDE_DEFAULT_PORT_PROPERTY;
+import static
org.apache.geode.internal.process.ProcessUtils.isProcessAlive;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.BindException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.junit.After;
+import org.junit.Before;
+
+import org.apache.geode.distributed.AbstractLauncher.Status;
+import org.apache.geode.internal.process.ProcessStreamReader;
+import org.apache.geode.internal.process.ProcessStreamReader.InputListener;
+
+/**
+ * Abstract base class for integration tests of {@link ServerLauncher} as
an application main in a
+ * forked JVM.
+ *
+ * @since GemFire 8.0
+ */
+public abstract class ServerLauncherRemoteIntegrationTestCase
+extends ServerLauncherIntegrationTestCase implements UsesServerCommand
{
+
+ private final AtomicBoolean threwBindException = new AtomicBoolean();
+
+ protected volatile Process process;
+ protected volatile ProcessStreamReader processOutReader;
+ protected volatile ProcessStreamReader processErrReader;
+
+ private ServerCommand serverCommand;
+
+ @Before
+ public void setUp() throws Exception {
+serverCommand = new ServerCommand(this);
+ }
+
+ @After
+ public void tearDownAbstractServerLauncherRemoteIntegrationTestCase()
throws Exception {
+if (this.process != null) {
+ this.process.destroy();
+ this.process = null;
+}
+if (this.processOutReader != null &&
this.processOutReader.isRunning()) {
+ this.processOutReader.stop();
+}
+if (this.processErrReader != null &&
this.processErrReader.isRunning()) {
+ this.processErrReader.stop();
+}
+ }
+
+ @Override
+ public List getJvmArguments() {
+List jvmArguments = new ArrayList<>();
+jvmArguments.add("-D" + GEMFIRE_PREFIX + LOG_LEVEL + "=config");
+jvmArguments.add("-D" + GEMFIRE_PREFIX + MCAST_PORT + "=0");
+jvmArguments
+.add("-D" + TEST_OVERRIDE_DEFAULT_PORT_PROPERTY + "=" +
String.valueOf(defaultServerPort));
+return jvmArguments;
+ }
+
+ @Override
+ public String getName() {
+return getUniqueName();
+ }
+
+ @Override
+ public boolean getDisableDefaultServer() {
+return false;
+ }
+
+ protected void assertThatServerThrewBindException() {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected void assertThatServerThrew(final Class
throwableClass) {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected ServerLauncher startServer(final ServerCommand command) {
+return awaitStart(command);
+ }
+
+ protected ServerLauncher awaitStart(final ServerCommand command,
+ final ProcessStreamReader.InputListener outListener,
+ final ProcessStreamReader.InputListener errListener) throws
Exception {
+executeCommandWithReaders(command.
[GitHub] geode pull request #699: GEODE-3413: overhaul launcher and process classes a...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132542242
--- 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 --
Done.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132542091
--- 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 --
All unused fields deleted.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132542146
--- 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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132542204
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTestCase.java
---
@@ -0,0 +1,270 @@
+/*
+ * 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.LOG_LEVEL;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static
org.apache.geode.distributed.internal.DistributionConfig.GEMFIRE_PREFIX;
+import static
org.apache.geode.internal.cache.AbstractCacheServer.TEST_OVERRIDE_DEFAULT_PORT_PROPERTY;
+import static
org.apache.geode.internal.process.ProcessUtils.isProcessAlive;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.BindException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.junit.After;
+import org.junit.Before;
+
+import org.apache.geode.distributed.AbstractLauncher.Status;
+import org.apache.geode.internal.process.ProcessStreamReader;
+import org.apache.geode.internal.process.ProcessStreamReader.InputListener;
+
+/**
+ * Abstract base class for integration tests of {@link ServerLauncher} as
an application main in a
+ * forked JVM.
+ *
+ * @since GemFire 8.0
+ */
+public abstract class ServerLauncherRemoteIntegrationTestCase
+extends ServerLauncherIntegrationTestCase implements UsesServerCommand
{
+
+ private final AtomicBoolean threwBindException = new AtomicBoolean();
+
+ protected volatile Process process;
+ protected volatile ProcessStreamReader processOutReader;
+ protected volatile ProcessStreamReader processErrReader;
+
+ private ServerCommand serverCommand;
+
+ @Before
+ public void setUp() throws Exception {
+serverCommand = new ServerCommand(this);
+ }
+
+ @After
+ public void tearDownAbstractServerLauncherRemoteIntegrationTestCase()
throws Exception {
+if (this.process != null) {
+ this.process.destroy();
+ this.process = null;
+}
+if (this.processOutReader != null &&
this.processOutReader.isRunning()) {
+ this.processOutReader.stop();
+}
+if (this.processErrReader != null &&
this.processErrReader.isRunning()) {
+ this.processErrReader.stop();
+}
+ }
+
+ @Override
+ public List getJvmArguments() {
+List jvmArguments = new ArrayList<>();
+jvmArguments.add("-D" + GEMFIRE_PREFIX + LOG_LEVEL + "=config");
+jvmArguments.add("-D" + GEMFIRE_PREFIX + MCAST_PORT + "=0");
+jvmArguments
+.add("-D" + TEST_OVERRIDE_DEFAULT_PORT_PROPERTY + "=" +
String.valueOf(defaultServerPort));
+return jvmArguments;
+ }
+
+ @Override
+ public String getName() {
+return getUniqueName();
+ }
+
+ @Override
+ public boolean getDisableDefaultServer() {
+return false;
+ }
+
+ protected void assertThatServerThrewBindException() {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected void assertThatServerThrew(final Class
throwableClass) {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected ServerLauncher startServer(final ServerCommand command) {
+return awaitStart(command);
+ }
+
+ protected ServerLauncher awaitStart(final ServerCommand command,
+ final ProcessStreamReader.InputListener outListener,
+ final ProcessStreamReader.InputListener errListener) throws
Exception {
+executeCommandWithReaders(command.
[GitHub] geode pull request #699: GEODE-3413: overhaul launcher and process classes a...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132542184
--- Diff:
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTestCase.java
---
@@ -0,0 +1,270 @@
+/*
+ * 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.LOG_LEVEL;
+import static
org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static
org.apache.geode.distributed.internal.DistributionConfig.GEMFIRE_PREFIX;
+import static
org.apache.geode.internal.cache.AbstractCacheServer.TEST_OVERRIDE_DEFAULT_PORT_PROPERTY;
+import static
org.apache.geode.internal.process.ProcessUtils.isProcessAlive;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.BindException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.junit.After;
+import org.junit.Before;
+
+import org.apache.geode.distributed.AbstractLauncher.Status;
+import org.apache.geode.internal.process.ProcessStreamReader;
+import org.apache.geode.internal.process.ProcessStreamReader.InputListener;
+
+/**
+ * Abstract base class for integration tests of {@link ServerLauncher} as
an application main in a
+ * forked JVM.
+ *
+ * @since GemFire 8.0
+ */
+public abstract class ServerLauncherRemoteIntegrationTestCase
+extends ServerLauncherIntegrationTestCase implements UsesServerCommand
{
+
+ private final AtomicBoolean threwBindException = new AtomicBoolean();
+
+ protected volatile Process process;
+ protected volatile ProcessStreamReader processOutReader;
+ protected volatile ProcessStreamReader processErrReader;
+
+ private ServerCommand serverCommand;
+
+ @Before
+ public void setUp() throws Exception {
+serverCommand = new ServerCommand(this);
+ }
+
+ @After
+ public void tearDownAbstractServerLauncherRemoteIntegrationTestCase()
throws Exception {
+if (this.process != null) {
+ this.process.destroy();
+ this.process = null;
+}
+if (this.processOutReader != null &&
this.processOutReader.isRunning()) {
+ this.processOutReader.stop();
+}
+if (this.processErrReader != null &&
this.processErrReader.isRunning()) {
+ this.processErrReader.stop();
+}
+ }
+
+ @Override
+ public List getJvmArguments() {
+List jvmArguments = new ArrayList<>();
+jvmArguments.add("-D" + GEMFIRE_PREFIX + LOG_LEVEL + "=config");
+jvmArguments.add("-D" + GEMFIRE_PREFIX + MCAST_PORT + "=0");
+jvmArguments
+.add("-D" + TEST_OVERRIDE_DEFAULT_PORT_PROPERTY + "=" +
String.valueOf(defaultServerPort));
+return jvmArguments;
+ }
+
+ @Override
+ public String getName() {
+return getUniqueName();
+ }
+
+ @Override
+ public boolean getDisableDefaultServer() {
+return false;
+ }
+
+ protected void assertThatServerThrewBindException() {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected void assertThatServerThrew(final Class
throwableClass) {
+assertThat(threwBindException.get()).isTrue();
+ }
+
+ protected ServerLauncher startServer(final ServerCommand command) {
+return awaitStart(command);
+ }
+
+ protected ServerLauncher awaitStart(final ServerCommand command,
+ final ProcessStreamReader.InputListener outListener,
+ final ProcessStreamReader.InputListener errListener) throws
Exception {
+executeCommandWithReaders(command.
[GitHub] geode issue #699: GEODE-3413: overhaul launcher and process classes and test...
Github user kirklund commented on the issue: https://github.com/apache/geode/pull/699 These tests have also now been overhauled or rewritten: * LauncherMemberMXBeanIntegrationTest * ServerLauncherWaitOnServerMultiThreadedTest * ServerLauncherWithProviderIntegrationTest -> ServerLauncherWithProviderRegressionTest --- 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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132545900
--- 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 --
Combined.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132592102
--- 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 --
Done.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132592382
--- 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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132598795
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/AbstractLauncher.java ---
@@ -795,25 +785,25 @@ protected String toString(final Date dateTime) {
// the value of a Number as a String, or "" if null
protected String toString(final Number value) {
- return StringUtils.defaultString(value);
+ return defaultString(value);
}
// a String concatenation of all values separated by " "
protected String toString(final Object... values) {
- return values == null ? "" : StringUtils.join(values, " ");
+ return values == null ? "" : join(values, " ");
}
// the value of the String, or "" if value is null
protected String toString(final String value) {
- return ObjectUtils.defaultIfNull(value, "");
+ return defaultIfNull(value, "");
}
}
/**
* The Status enumerated type represents the various lifecycle states of
a GemFire service (such
* as a Cache Server, a Locator or a Manager).
*/
- public static enum Status {
--- End diff --
It's static with or without the keyword. IntelliJ grays it out so I just
deleted it. Similar to public on methods defined in an Interface.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132598899
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java ---
@@ -104,6 +109,8 @@
helpMap.put("bind-address",
--- End diff --
I'd love to break these Launcher classes up into smaller classes. I'd also
like to move them from org.apache.geode.distributed -- I don't think that pkg
really makes sense for these classes to live 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 #699: GEODE-3413: overhaul launcher and process classes a...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132599242
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java ---
@@ -1352,11 +1328,13 @@ protected void parseCommand(final String... args) {
* @see
org.apache.geode.distributed.LocatorLauncher.Command#isCommand(String)
* @see #parseArguments(String...)
*/
-protected void parseMemberName(final String[] args) {
- for (String arg : args) {
-if (!(arg.startsWith(OPTION_PREFIX) || Command.isCommand(arg))) {
- setMemberName(arg);
- break;
+protected void parseMemberName(final String... args) {
+ if (args != null) {
+for (String arg : args) {
+ if (!(arg.startsWith(OPTION_PREFIX) || Command.isCommand(arg))) {
--- End diff --
We need to refactor these classes a lot more. I only copied line 1332 from
ServerLauncher which then indented the rest of the lines. I made sure that
everything was more consistent. LocatorLauncher threw NPE if args was null but
ServerLauncher was ok with null.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132599333
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java ---
@@ -1781,8 +1782,8 @@ protected void validate() {
* @see org.apache.geode.distributed.LocatorLauncher.Command#START
*/
protected void validateOnStart() {
- if (Command.START.equals(getCommand())) {
-if (StringUtils.isBlank(getMemberName())
+ if (Command.START == getCommand()) {
--- End diff --
Yep, IntelliJ has an automated refactoring and inspection for this. I don't
even remember making this change unless I was trying to make LocatorLauncher
more consistent with ServerLauncher.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132599627
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/process/AttachProcessUtils.java
---
@@ -14,21 +14,28 @@
*/
package org.apache.geode.internal.process;
-import org.apache.geode.internal.process.ProcessUtils.InternalProcessUtils;
+import static org.apache.commons.lang.Validate.isTrue;
+
import com.sun.tools.attach.VirtualMachine;
import com.sun.tools.attach.VirtualMachineDescriptor;
+import org.apache.geode.internal.process.ProcessUtils.InternalProcessUtils;
+
/**
* Implementation of the {@link ProcessUtils} SPI that uses the JDK Attach
API.
*
* @since GemFire 8.0
*/
class AttachProcessUtils implements InternalProcessUtils {
- AttachProcessUtils() {}
+ AttachProcessUtils() {
--- End diff --
Deleted ctor from AttachProcessUtils and NativeProcessUtils.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132599768
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/process/FileProcessController.java
---
@@ -112,56 +119,43 @@ private void stop(final File workingDir, final String
stopRequestFileName) throw
private String status(final File workingDir, final String
statusRequestFileName,
final String statusFileName) throws IOException,
InterruptedException, TimeoutException {
// monitor for statusFile
-final File statusFile = new File(workingDir, statusFileName);
-final AtomicReference statusRef = new AtomicReference<>();
-
-final ControlRequestHandler statusHandler = new
ControlRequestHandler() {
- @Override
- public void handleRequest() throws IOException {
-// read the statusFile
-final BufferedReader reader = new BufferedReader(new
FileReader(statusFile));
-final StringBuilder lines = new StringBuilder();
-try {
- String line = null;
- while ((line = reader.readLine()) != null) {
-lines.append(line);
- }
-} finally {
- statusRef.set(lines.toString());
- reader.close();
+File statusFile = new File(workingDir, statusFileName);
+AtomicReference statusRef = new AtomicReference<>();
+
+ControlRequestHandler statusHandler = () -> {
--- End diff --
Done.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132599897
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/process/LocalProcessLauncher.java
---
@@ -96,33 +117,55 @@ void close() {
* @throws IOException if unable to create or write to the file
*/
private void writePid(final boolean force) throws
FileAlreadyExistsException, IOException {
-final boolean created = this.pidFile.createNewFile();
-if (!created && !force) {
- int otherPid = 0;
- try {
-otherPid = ProcessUtils.readPid(this.pidFile);
- } catch (IOException e) {
-// suppress
- } catch (NumberFormatException e) {
-// suppress
- }
- boolean ignorePidFile = false;
- if (otherPid != 0 && !ignoreIsPidAlive()) {
-ignorePidFile = !ProcessUtils.isProcessAlive(otherPid);
- }
- if (!ignorePidFile) {
-throw new FileAlreadyExistsException("Pid file already exists: " +
this.pidFile + " for "
-+ (otherPid > 0 ? "process " + otherPid : "unknown process"));
+if (this.pidFile.exists()) {
+ if (!force) {
+checkOtherPid(readOtherPid());
}
+ this.pidFile.delete();
+}
+
+assert !this.pidFile.exists();
--- End diff --
Did you like the use of Validate? I'm not sure how I feel about Validate
after using it heavily in this package.
Jianxia added the asserts when he fixed a bug in this method and I just
left them in place. I think we could easily change them to Validate calls or
remove them altogether.
---
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 kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132600143
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/process/LocalProcessLauncher.java
---
@@ -96,33 +117,55 @@ void close() {
* @throws IOException if unable to create or write to the file
*/
private void writePid(final boolean force) throws
FileAlreadyExistsException, IOException {
-final boolean created = this.pidFile.createNewFile();
-if (!created && !force) {
- int otherPid = 0;
- try {
-otherPid = ProcessUtils.readPid(this.pidFile);
- } catch (IOException e) {
-// suppress
- } catch (NumberFormatException e) {
-// suppress
- }
- boolean ignorePidFile = false;
- if (otherPid != 0 && !ignoreIsPidAlive()) {
-ignorePidFile = !ProcessUtils.isProcessAlive(otherPid);
- }
- if (!ignorePidFile) {
-throw new FileAlreadyExistsException("Pid file already exists: " +
this.pidFile + " for "
-+ (otherPid > 0 ? "process " + otherPid : "unknown process"));
+if (this.pidFile.exists()) {
+ if (!force) {
+checkOtherPid(readOtherPid());
}
+ this.pidFile.delete();
+}
+
+assert !this.pidFile.exists();
--- End diff --
I deleted 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.
---
[GitHub] geode issue #699: GEODE-3413: overhaul launcher and process classes and test...
Github user kirklund commented on the issue: https://github.com/apache/geode/pull/699 Merged this PR to develop. Done! --- 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 kirklund closed the pull request at: https://github.com/apache/geode/pull/699 --- 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 #711: GEODE-3436: revert all commands refactorings
GitHub user kirklund opened a pull request: https://github.com/apache/geode/pull/711 GEODE-3436: revert all commands refactorings Revert these commits to return precheckin to GREEN: commit d27f8b956de7d9c5d95ebdc68dfc67ee8b2d7b51 Author: YehEmily Date: Mon Aug 7 13:09:42 2017 -0700 GEODE-3264: Refactoring MemberCommands This closes #692 commit 440c87f81fab96f9ce38a2d53ded75e5fe8390d7 Author: YehEmily Date: Mon Aug 7 11:52:14 2017 -0700 GEODE-3259: Refactoring DurableClientCommands This closes #689 commit 97c4e9a59f17c7bc914e39dd048b0a4cd96293c4 Author: YehEmily Date: Wed Jul 26 11:07:09 2017 -0700 GEODE-3254: Refactoring ConfigCommands This closes #665 commit ed293e817e547fb5ecd399bf4ba10d694af51e0a Author: YehEmily Date: Mon Aug 7 12:35:14 2017 -0700 GEODE-3262: Refactoring IndexCommands This closes #690 commit 90f5440de8ec747f301a309a0a34101e8defcd29 Author: YehEmily Date: Mon Aug 7 12:56:17 2017 -0700 GEODE-3260: Refactoring FunctionCommands This closes #691 commit 5d6cad7755ec3c4fe931e3d0f8e89fb181038543 Author: YehEmily Date: Thu Aug 3 09:00:08 2017 -0700 GEODE-3258: Refactoring DiskStoreCommands This closes #687 commit 210ff9f15460c993f2bf7fd682d50ee65462cd23 Author: YehEmily Date: Fri Aug 11 10:22:33 2017 -0700 GEODE-3337: Refactoring LauncherLifecycleCommandsDUnitTest This closes #701 commit 63169699e933f6e0fdd90b95ed039e4e3c92c32c Author: YehEmily Date: Mon Aug 7 15:37:23 2017 -0700 GEODE-3265: Refactoring MiscellaneousCommands This closes #696 commit cf91426692349d0c81ce77394935576d9cc336e8 Author: YehEmily Date: Fri Aug 4 11:12:50 2017 -0700 GEODE-3261: Refactoring GfshHelpCommands This closes #685 commit fd47ed660168864a6f81b2a4cd7dbceebc99a282 Author: YehEmily Date: Mon Aug 7 14:47:15 2017 -0700 GEODE-3267: Refactoring QueueCommands This closes #695 commit 359e3fff6482ecfb375939d387f4dad3a636246b Author: YehEmily Date: Mon Aug 7 14:32:43 2017 -0700 GEODE-3270: Refactoring (renaming) StatusCommands This closes #694 commit 957d583e54dc34c029885f32a54f0b25a3ac1094 Author: YehEmily Date: Mon Aug 7 15:25:24 2017 -0700 GEODE-3267: Refactoring QueueCommands - updated based on feedback commit 64de3b69c2aecb4930bcfd0a1161569b1d5fda89 Author: YehEmily Date: Mon Aug 7 13:58:08 2017 -0700 GEODE-3268: Refactoring RegionCommands This closes #693 commit 67185abcdd68b908dea6888cb94286b8aa9ea49f Author: YehEmily Date: Fri Aug 4 10:47:48 2017 -0700 GEODE-3266: Refactoring PDXCommands This closes #684 commit 9d967446a44a78b612f605b6a8f8eedcfc625b3a Author: YehEmily Date: Wed Aug 2 17:28:10 2017 -0700 GEODE-3257: Refactoring DeployCommands This closes #680 commit 756efe77c86bb03ac9984655e7bd040659e85890 Author: YehEmily Date: Fri Jul 28 14:23:25 2017 -0700 GEODE-3255: Refactor CreateAlterDestroyRegionCommands and tests This closes #671 commit a7f29525df2981c1c99abac96ea83cb965295970 Author: YehEmily Date: Fri Jul 21 14:29:55 2017 -0700 GEODE-3230: Cleaning up unused (Cli)Strings This closes #651 You can merge this pull request into a Git repository by running: $ git pull https://github.com/kirklund/geode revert-all Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/711.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 #711 commit 47377c8086e57a88a87244ed77cad48e43495dd5 Author: Kirk Lund Date: 2017-08-12T16:38:20Z Revert "GEODE-3264: Refactoring MemberCommands" This reverts commit d27f8b956de7d9c5d95ebdc68dfc67ee8b2d7b51. commit fd6fcc7dd65b83c030d6f7bb754ed921a9ab8829 Author: Kirk Lund Date: 2017-08-12T16:38:28Z Revert "GEODE-3259: Refactoring DurableClientCommands" This reverts commit 440c87f81fab96f9ce38a2d53ded75e5fe8390d7. commit 969d71b60fa3d8ff8b91a21d8c4df4a830cfec30 Author: Kirk Lund Date: 2017-08-12T16:38:29Z Revert "GEODE-3254: Refactoring ConfigCommands" This reverts commit 97c4e9a59f17c7bc914e39dd048b0a4cd96293c4. commit d0e0e63a4842bf17af808b1c5dc8d5e04dcb0cac Author: Kirk Lund Date: 2017-08-12T16:
[GitHub] geode issue #613: GEODE-3151: Internal Region Registration in JMX as per con...
Github user kirklund commented on the issue: https://github.com/apache/geode/pull/613 I think this sort of change should be proposed and discussed on the dev list. This is effectively adding new User APIs and that requires more discussion. --- 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 #720: GEODE-3462: FunctionStats Exposed over JMX
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/720#discussion_r134003148
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
---
@@ -3091,4 +3068,22 @@ public void stopReconnecting() {
Throwable generateCreationStack(final DistributionConfig config);
}
+
+ public void requestRegisterBean(Function function) {
--- End diff --
DistributedSystem should limit its concerns to membership and clustering.
These are new methods for MBeans and Functions -- InternalDistributedSystem
should not care about MBeans and Functions. ManagementService already has some
User APIs for MBeans.
---
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 #720: GEODE-3462: FunctionStats Exposed over JMX
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/720#discussion_r134004793
--- Diff:
geode-core/src/main/java/org/apache/geode/management/FunctionStatsMXBean.java
---
@@ -0,0 +1,54 @@
+/*
+ * 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;
+
+import org.apache.geode.cache.asyncqueue.AsyncEventQueue;
+import org.apache.geode.management.internal.security.ResourceOperation;
+import org.apache.geode.security.ResourcePermission.Operation;
+import org.apache.geode.security.ResourcePermission.Resource;
+
+/**
+ * MBean that provides access to an {@link AsyncEventQueue}.
+ *
+ * @since GemFire 7.0
+ *
+ */
+@ResourceOperation(resource = Resource.CLUSTER, operation = Operation.READ)
+public interface FunctionStatsMXBean {
--- End diff --
The design of this MBean doesn't fit in with the other Geode MBeans. We are
not creating "stats" mbeans, we are creating mbeans for components in Geode.
Example: we have a CacheServerMXBean and some of the attributes on that mbean
are CacheServer stats.
I would recommend writing up some sort of proposal for adding a new
FunctionService mbean on the Apache Geode wiki and then starting a discussion
about this on the dev list. However, I should note that we've purposely avoided
fine-grained mbeans, preferring overall mbeans such as FunctionServiceMXBean
instead of having an mbean per Function. The same issue came up with Clients
and Geode dev community decided that having an mbean per Client was NOT the
direction we wanted to go in -- you may be going down a similar path here.
At present, the Cache and Function service are both squashed on the
MemberMXBean which is why there is no separate mbean for Function service.
---
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 #720: GEODE-3462: FunctionStats Exposed over JMX
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/720#discussion_r134003423
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
---
@@ -3138,7 +3138,7 @@ public Pool determineDefaultPool(PoolFactory
poolFactory) {
invokeRegionAfter(region);
// Added for M&M . Putting the callback here to avoid creating
RegionMBean in case of Exception
-if (!region.isInternalRegion()) {
+if (region.isRegionRegistrationRequireOnJmx()) {
--- End diff --
As a community we are trying to refactor code and separate concerns. This
change is going in the wrong direction -- you're combining JMX concerns into
Cache and Region which is beyond the scope of these classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #720: GEODE-3462: FunctionStats Exposed over JMX
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/720#discussion_r134003701
--- Diff:
geode-core/src/main/java/org/apache/geode/management/FunctionStatsMXBean.java
---
@@ -0,0 +1,54 @@
+/*
+ * 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;
+
+import org.apache.geode.cache.asyncqueue.AsyncEventQueue;
+import org.apache.geode.management.internal.security.ResourceOperation;
+import org.apache.geode.security.ResourcePermission.Operation;
+import org.apache.geode.security.ResourcePermission.Resource;
+
+/**
+ * MBean that provides access to an {@link AsyncEventQueue}.
+ *
+ * @since GemFire 7.0
--- End diff --
This is Geode, not GemFire and it certainly isn't 7.0. The next release
would be Geode 1.3.0 but we really aren't supposed to add new User APIs in
minor releases -- it may have to wait for Geode 2.0 -- that's something to
discuss on the dev list 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 #724: GEODE-3469: prevent zero pid from AvailablePid for ...
GitHub user kirklund opened a pull request: https://github.com/apache/geode/pull/724 GEODE-3469: prevent zero pid from AvailablePid for tests You can merge this pull request into a Git repository by running: $ git pull https://github.com/kirklund/geode GEODE-3469-LocatorLauncherLocalFileIntegrationTest Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/724.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 #724 commit 53b98edb1fb6340c9dfc5921317ee129fdcf4ff4 Author: Kirk Lund Date: 2017-08-18T21:33:25Z GEODE-3469: prevent zero pid from AvailablePid for 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 #724: GEODE-3469: prevent zero pid from AvailablePid for ...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/724#discussion_r134077082
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/process/lang/AvailablePid.java
---
@@ -30,37 +30,88 @@
*/
public class AvailablePid {
- static final int LOWER_BOUND = 1;
- static final int UPPER_BOUND = 64000;
+ static final int DEFAULT_LOWER_BOUND = 1;
+ static final int DEFAULT_UPPER_BOUND = 64000;
static final int DEFAULT_TIMEOUT_MILLIS = 60 * 1000;
+ private final int lowerBound;
+ private final int upperBound;
private final Random random;
private final int timeoutMillis;
/**
- * Construct with no seed and default timeout of 1 minute.
+ * Construct with:
+ *
+ * default {@link Bounds} of {@link #DEFAULT_LOWER_BOUND}
(inclusive) and
+ * {@link #DEFAULT_UPPER_BOUND} (inclusive)
+ * Random with no see
--- End diff --
I no see your point ;)
---
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 #727: GEODE-3430: fix varargs usage
GitHub user kirklund opened a pull request: https://github.com/apache/geode/pull/727 GEODE-3430: fix varargs usage Also, general cleanup of ConnectCommandTest. You can merge this pull request into a Git repository by running: $ git pull https://github.com/kirklund/geode GEODE-3430-ConnectCommandTest Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/727.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 #727 commit cbbb597cfc5e3dd8610fbb29d7df3875c02b49e5 Author: Kirk Lund Date: 2017-08-21T17:12:08Z GEODE-3430: fix varargs usage Also, general cleanup of ConnectCommandTest. --- 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 #724: GEODE-3469: prevent zero pid from AvailablePid for ...
Github user kirklund closed the pull request at: https://github.com/apache/geode/pull/724 --- 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 #724: GEODE-3469: prevent zero pid from AvailablePid for tests
Github user kirklund commented on the issue: https://github.com/apache/geode/pull/724 Already merged 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 #729: GEODE-3461: increase test timeouts
GitHub user kirklund opened a pull request: https://github.com/apache/geode/pull/729 GEODE-3461: increase test timeouts You can merge this pull request into a Git repository by running: $ git pull https://github.com/kirklund/geode GEODE-3461-increase-timeouts Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/729.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 #729 commit 88e7ca09e342276efa0af596cbd7de3bc96a4c1a Author: Kirk Lund Date: 2017-08-21T22:38:45Z GEODE-3461: increase test timeouts --- 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 #729: GEODE-3461: increase test timeouts
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/729#discussion_r134352194
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/process/AbstractProcessStreamReaderIntegrationTest.java
---
@@ -149,7 +147,7 @@ protected void
givenStartedProcessWithStreamListeners(final Class mainClass)
}
protected ConditionFactory await() {
-return
Awaitility.await().atMost(WAIT_FOR_READER_IS_RUNNING_TIMEOUT_MILLIS,
MILLISECONDS);
+return Awaitility.await().atMost(2, MINUTES);
--- End diff --
WAIT_FOR_READER_IS_RUNNING_TIMEOUT_MILLIS was poorly named and is now
deleted --
replaced by (2, MINUTES) hardcoded into the await() method that various
tests use.
READER_JOIN_TIMEOUT_MILLIS still exists and is used only by the two tests
that perform Thread.join style calls on the Readers.
Both timeouts are now 2 minutes which should be more than enough even if
the test hits a GC pause.
---
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 #729: GEODE-3461: increase test timeouts
Github user kirklund commented on the issue: https://github.com/apache/geode/pull/729 This PR should also fix GEODE-3505. --- 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. ---
