[GitHub] geode issue #528: GEODE-1994, revisited: Removed guaranteed failures.
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/528 LGTM (pending green precheckin) --- 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 #528: GEODE-1994, revisited: Removed guaranteed failures.
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/528 Merged as dff937f --- 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 #539: GEODE-2818: add alias to any command's options that involv...
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/539 Thanks for your contribution, Emily! It looks like this branch will need to be rebased onto the latest develop to apply cleanly. Would you mind updating the PR with a rebased version of these changes? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/549#discussion_r119220596
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
---
@@ -1794,18 +1801,20 @@ public Result statusServer(
.format(CliStrings.STATUS_SERVICE__GFSH_NOT_CONNECTED_ERROR_MESSAGE, "Cache
Server"));
}
} else {
-final ServerLauncher serverLauncher = new ServerLauncher.Builder()
-
.setCommand(ServerLauncher.Command.STATUS).setDebug(isDebugging())
-// NOTE since we do not know whether the "CacheServer" was
enabled or not on the GemFire
-// server when it was started, set the disableDefaultServer
property in the
-// ServerLauncher.Builder to default status
-// to the MemberMBean
-// TODO fix this hack! (how, the 'start server' loop needs it)
-
.setDisableDefaultServer(true).setPid(pid).setWorkingDirectory(workingDirectory)
-.build();
-
+final ServerLauncher serverLauncher;
+if ((member == null)) {
--- End diff --
It looks like we're guaranteed that `member` is not blank in this `else`
block, so I think we are guaranteed that `member` is not null here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/549#discussion_r119219867
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
---
@@ -771,11 +771,18 @@ public Result statusLocator(
CliStrings.STATUS_SERVICE__GFSH_NOT_CONNECTED_ERROR_MESSAGE,
LOCATOR_TERM_NAME));
}
} else {
-final LocatorLauncher locatorLauncher =
-new
LocatorLauncher.Builder().setCommand(LocatorLauncher.Command.STATUS)
-
.setBindAddress(locatorHost).setDebug(isDebugging()).setPid(pid)
-
.setPort(locatorPort).setWorkingDirectory(workingDirectory).build();
+final LocatorLauncher locatorLauncher;
+if ((locatorHost == null) && (locatorPort == null) &&
(workingDirectory == null)) {
--- End diff --
What if `locatorPort` is specified, but `locatorHost` is not? It seems
like that might pass this check, but still result in an error.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/549#discussion_r119229190
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
---
@@ -771,11 +771,18 @@ public Result statusLocator(
CliStrings.STATUS_SERVICE__GFSH_NOT_CONNECTED_ERROR_MESSAGE,
LOCATOR_TERM_NAME));
}
} else {
-final LocatorLauncher locatorLauncher =
-new
LocatorLauncher.Builder().setCommand(LocatorLauncher.Command.STATUS)
-
.setBindAddress(locatorHost).setDebug(isDebugging()).setPid(pid)
-
.setPort(locatorPort).setWorkingDirectory(workingDirectory).build();
+final LocatorLauncher locatorLauncher;
+if ((locatorHost == null) && (locatorPort == null) &&
(workingDirectory == null)) {
--- End diff --
I had thought that `locatorPort` would not uniquely identify a locator
without also specifying a `locatorHost`, but it looks like we might end up
falling back to a default value of localhost. It would be nice to add a test
that make sure this works as expected. (I can pair with you on that if you'd
like!)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Github user jaredjstewart commented on a diff in the pull request: https://github.com/apache/geode/pull/549#discussion_r119231037 --- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java --- @@ -2747,6 +2749,8 @@ public static final String STATUS_SERVER__DIR = "dir"; public static final String STATUS_SERVER__DIR__HELP = "Working directory in which the Cache Server is running. The default is the current directory."; + public static final String STATUS_SERVER__NO_SERVER_SPECIFIED_ERROR_MESSAGE = --- End diff -- I believe the `running in %2$s` portion of this output is going to be misleading when the status locator command has omitted the `workingDirectory` option. For example, if I ask for the status of a remote locator by specifying its hostname and port, this message will suggest that we expect the locator to be running in the current directory on the GFSH client machine. It might also be helpful for this message to explain *why* we are unable to determine the status of this Locator. Maybe something like "At least one of the following options must be specified: `--name`, `--host`, or `--port`. Use 'help status server' to display detailed usage information." would be more helpful. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/549#discussion_r119233486
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
---
@@ -771,11 +771,18 @@ public Result statusLocator(
CliStrings.STATUS_SERVICE__GFSH_NOT_CONNECTED_ERROR_MESSAGE,
LOCATOR_TERM_NAME));
}
} else {
-final LocatorLauncher locatorLauncher =
-new
LocatorLauncher.Builder().setCommand(LocatorLauncher.Command.STATUS)
-
.setBindAddress(locatorHost).setDebug(isDebugging()).setPid(pid)
-
.setPort(locatorPort).setWorkingDirectory(workingDirectory).build();
+final LocatorLauncher locatorLauncher;
+if ((locatorHost == null) && (locatorPort == null) &&
(workingDirectory == null)) {
--- End diff --
Looking into this a bit further, I think we probably will need to add `&&
(pid == 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 #551: GEODE-2971: consistency in shell exit codes for sta...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/551#discussion_r119419341
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/ShellExitCode.java ---
@@ -0,0 +1,40 @@
+/*
+ * 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;
+
+public class ShellExitCode {
--- End diff --
This should be an `enum` rather than a `class`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #551: GEODE-2971: consistency in shell exit codes for sta...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/551#discussion_r119420029
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/ShellExitCode.java ---
@@ -0,0 +1,40 @@
+/*
+ * 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;
+
+public class ShellExitCode {
--- End diff --
E.g.
```
public enum ShellExitCode {
NORMAL(0),FATAL(1), INVALID(1);
private MyEnum(int exitCode) {
this.exitCode = exitCode;
}
private int exitCode;
public int getExitCode(){
return this.exitCode;
}
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #530: GEODE-2981: Fix the line feed code of the test expected va...
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/530 Thank you for your contribution, looks good to me! --- 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 #557: add GfshParserRule to facilitate command unit test
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/557 Looks like travis-ci failed. LGTM as soon as travis passes --- 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 #508: GEODE-2908: Adding the new tag as per swagger notification...
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/508 Looks good to me. I'll merge it in shortly and cherry pick onto the release branch. --- 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 #566: GEODE-3044: User Manual: Update Swagger example and screen...
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/566 I think it would probably be best to omit `--J=-Dgemfire.http-service-bind-address=localhost` from the `start server` command for the sake of simplicity. You also might want to consider changing `/Users/dbarnes/apache-geode-1.2.0/locator1` to something more generic like `/Users/admin/apache-geode-1.2.0/locator1` in the command output. Looks good otherwise! --- 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 #566: GEODE-3044: User Manual: Update Swagger example and screen...
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/566 Some more feedback after walking through the example.. Now that we start a locator (which starts an http server of its own on 7070) we need to specify a different http port for the server to use. Here is the command that I ended up using: ``` start server --name=server1 --start-rest-api --J=-Dgemfire.jmx-manager=true --J=-Dgemfire.jmx-manager-start=true --J=-Dgemfire.http-service-port=8080 ``` --- 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 #566: GEODE-3044: User Manual: Update Swagger example and screen...
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/566 (I also need to file a bug since that command appeared to hang.. But it did end up starting the server despite gfsh hanging with an endless ".") --- 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 #508: GEODE-2908: Adding the new tag as per swagger notification...
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/508 Merged as `ac404ad`. Thank you for your contribution! --- 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 #508: GEODE-2908: Adding the new tag as per swagger notification...
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/508 This PR has already been merged in, but feel free to open a new ticket for that. We unfortunately still have a lot of other "gemfire" references scattered around. --- 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 jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/580#discussion_r122066276
--- Diff:
geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparator.java
---
@@ -228,4 +139,55 @@ void addEvaluatedSortCriteria(Object row,
ExecutionContext context)
// No op
}
+ private int compareHelperMethod(Object obj1, Object obj2) {
+if (obj1 == null || obj2 == null) {
+ return compareIfOneOrMoreNull(obj1, obj2);
+} else if (obj1 == QueryService.UNDEFINED || obj2 ==
QueryService.UNDEFINED) {
+ return compareIfOneOrMoreQueryServiceUndefined(obj1, obj2);
+} else {
+ return compareTwoObjects(obj1, obj2);
+}
+ }
+
+ private int compareIfOneOrMoreNull(Object obj1, Object obj2) {
+if (obj1 == null) {
+ return obj2 == null ? 0 : -1;
+} else {
+ return 1;
+}
+ }
+
+ private int compareIfOneOrMoreQueryServiceUndefined(Object obj1, Object
obj2) {
+if (obj1 == QueryService.UNDEFINED) {
+ return obj2 == QueryService.UNDEFINED ? 0 : -1;
+} else {
+ return 1;
+}
+ }
+
+ private int compareTwoObjects(Object obj1, Object obj2) {
+if (obj1 instanceof Number && obj2 instanceof Number) {
+ return compareTwoNumbers(obj1, obj2);
+} else {
+ return compareTwoStrings(obj1, obj2);
+}
+ }
+
+ private int compareTwoNumbers(Object obj1, Object obj2) {
--- End diff --
I see it was not introduced by your changes, but the subtraction on 177 is
not a safe way to compare doubles as it can result in under/overflow. For
example, consider the result of `compareTwoNumbers( Double.MAX_VALUE, -1d)`.
Here's a safer way to do it:
```
private int compareTwoNumbers(Object obj1, Object obj2) {
Number number1 = (Number) obj1;
Number number2 = (Number) obj2 ;
return Double.compare(number1.doubleValue(), number2.doubleValue());
}
```
---
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 #578: GEODE-1958: Removing/deprecating PasswordUtil
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/578#discussion_r122073349
--- Diff:
geode-core/src/test/java/org/apache/geode/cache/util/PasswordUtilJUnitTest.java
---
@@ -1,42 +0,0 @@
-/*
- * 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.cache.util;
-
-import static org.junit.Assert.*;
-
-import org.junit.Test;
-import org.junit.experimental.categories.Category;
-
-import org.apache.geode.internal.util.PasswordUtil;
-import org.apache.geode.test.junit.categories.SecurityTest;
-import org.apache.geode.test.junit.categories.UnitTest;
-
-@Category({UnitTest.class, SecurityTest.class})
-public class PasswordUtilJUnitTest {
-
- @Test
- public void testPasswordUtil() {
-String x = "password";
-String z = null;
-
-// System.out.println(x);
-String y = PasswordUtil.encrypt(x);
--- End diff --
I think we ought to keep this test around until we actually remove
`decrypt()`. Since `encrypt()` won't be around to us to use in the test, I
think it would suffice to hardcode a String that we know ought to be decrypted
to some known value (basically grab the value of `y` that results when you run
this test on `Develop`). Then we can make sure that we haven't accidentally
broken the behavior of `decrypt()` with this change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #596: GEODE-2920 - GEODE-2925: Finer Grained Security
Github user jaredjstewart commented on a diff in the pull request: https://github.com/apache/geode/pull/596#discussion_r123815346 --- Diff: geode-core/src/main/java/org/apache/geode/management/CacheServerMXBean.java --- @@ -60,48 +61,48 @@ /** * Returns the port on which this CacheServer listens for clients. */ - public int getPort(); + int getPort(); --- End diff -- Why are we removing public from all of these methods? I think CacheServerMXBean is a public facing API and intends to expose these 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 #596: GEODE-2920 - GEODE-2925: Finer Grained Security
Github user jaredjstewart commented on a diff in the pull request: https://github.com/apache/geode/pull/596#discussion_r123817037 --- Diff: geode-core/src/main/java/org/apache/geode/management/CacheServerMXBean.java --- @@ -60,48 +61,48 @@ /** * Returns the port on which this CacheServer listens for clients. */ - public int getPort(); + int getPort(); --- End diff -- I think this should be ok actually, since CacheServerMXBean is an interface and not a class. (So all methods will be public by default.) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #596: GEODE-2920 - GEODE-2925: Finer Grained Security
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/596#discussion_r123818520
--- Diff:
geode-core/src/test/java/org/apache/geode/management/internal/security/DiskStoreMXBeanSecurityJUnitTest.java
---
@@ -57,7 +56,48 @@ public void setUp() throws Exception {
}
@Test
- @ConnectionConfiguration(user = "data-admin", password = "1234567")
+ @ConnectionConfiguration(user = "clusterRead", password = "clusterRead")
+ public void testClusterReadAccess() throws Exception {
+assertThatThrownBy(() ->
bean.flush()).hasMessageContaining(TestCommand.diskManage.toString());
--- End diff --
A bunch of tests in this change set can be simplified by using method
references in place of lambda expressions (when the lambdas take no parameters
and invoke a no-arg method on the target):
```
assertThatThrownBy(bean::flush).hasMessageContaining(TestCommand.diskManage.toString());
```
in place of
```
assertThatThrownBy(() ->
bean.flush()).hasMessageContaining(TestCommand.diskManage.toString());
```
---
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 #596: GEODE-2920 - GEODE-2925: Finer Grained Security
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/596#discussion_r123819260
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBean.java
---
@@ -148,7 +148,12 @@ public long getInitialImageTime() {
@Override
public int getInitialImagesInProgres() {
--- End diff --
We ended up with two copies of this method, one with the correct spelling
and one with the incorrect spelling.
---
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 #596: GEODE-2920 - GEODE-2925: Finer Grained Security
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/596#discussion_r123820699
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ClientCommands.java
---
@@ -109,12 +107,10 @@ public Result listClient() {
}
String memberSeparator = "; ";
- Iterator>> it =
clientServerMap.entrySet().iterator();
- while (it.hasNext()) {
-Map.Entry> pairs = (Map.Entry>) it.next();
-String client = (String) pairs.getKey();
-List servers = (List) pairs.getValue();
+ for (Entry> pairs : clientServerMap.entrySet())
{
+String client = pairs.getKey();
+List servers = pairs.getValue();
StringBuilder serverListForClient = new StringBuilder();
--- End diff --
Since we're already cleaning up this section of the code, it might be worth
considering replacing 114-123 with a one liner:
```
String serverListForClient =
servers.stream().collect(Collectors.joining(memberSeparator));
```
---
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 #596: GEODE-2920 - GEODE-2925: Finer Grained Security
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/596#discussion_r123822645
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/FunctionCommands.java
---
@@ -130,31 +125,8 @@ public Result executeFunction(
return result;
}
- if (onRegion != null && onMember != null && onGroups != null) {
-ErrorResultData errorResultData =
-
ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT)
-.addLine(CliStrings.EXECUTE_FUNCTION__MSG__OPTIONS);
-result = ResultBuilder.buildResult(errorResultData);
-return result;
- } else if (onRegion != null && onMember != null) {
-ErrorResultData errorResultData =
-
ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT)
-.addLine(CliStrings.EXECUTE_FUNCTION__MSG__OPTIONS);
-result = ResultBuilder.buildResult(errorResultData);
-return result;
- } else if (onMember != null && onGroups != null) {
-ErrorResultData errorResultData =
-
ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT)
-.addLine(CliStrings.EXECUTE_FUNCTION__MSG__OPTIONS);
-result = ResultBuilder.buildResult(errorResultData);
-return result;
- } else if (onRegion != null && onGroups != null) {
-ErrorResultData errorResultData =
-
ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT)
-.addLine(CliStrings.EXECUTE_FUNCTION__MSG__OPTIONS);
-result = ResultBuilder.buildResult(errorResultData);
-return result;
- } else if (onRegion != null && onMember != null && onGroups != null)
{
+ if (isMoreThanOneIsTrue(onRegion != null, onMember != null, onGroups
!= null)) {
--- End diff --
I like the readability introduced by this `isMoreThanOneIsTrue` method. It
might make sense to take the abstraction one step further:
```
if (isMoreThanOneNonNull(onRegion, onMember, onGroup) {
```
```
private boolean isMoreThanOneNonNull(Object... values) {
return Stream.of(values).filter(Objects::nonNull).count() > 1;
}
```
---
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 #596: GEODE-2920 - GEODE-2925: Finer Grained Security
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/596 +1 --- 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 #596: GEODE-2920 - GEODE-2925: Finer Grained Security
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/596 Merged as 451d12e --- 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 #652: Geode-2971: Introduce ExitCode to resolve inconsistency of...
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/652 To answer one of your `//TODO` questions, I think `gfsh help` should return a normal exit code. Can you collect the other questions you have and post them in this PR? It will be much easier to make sure I've answered them all than to dig through the changes looking for TODOs. Thanks! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #651: GEODE-3230: Cleaning up unused (Cli)Strings
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/651#discussion_r129647633
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommands.java
---
@@ -639,29 +639,34 @@ public Result compactOfflineDiskStore(
String errorString = CliStrings.format(
CliStrings.COMPACT_OFFLINE_DISK_STORE__MSG__ERROR_WHILE_COMPACTING_DISKSTORE_0_WITH_1_REASON_2,
diskStoreName, fieldsMessage);
- result = ResultBuilder.createUserErrorResult(errorString);
+ result = ResultBuilder.createUserErrorResult(CliStrings.format(
--- End diff --
This looks to me like it's going to build a strange error message that I
expect will look something like this::
```
An error occurred while doing compaction: "While compacting disk store={0}
{1}. Reason: {2}"
```
I think {0} and {1} will be filled in but not {2}.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #664: Fix for GEODE-3292
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/664 Hi Juan, Thanks for your contribution! Your changes look good to me, but our [Criteria for Code Submissions](https://cwiki.apache.org/confluence/display/GEODE/Criteria+for+Code+Submissions) require having tests for the behavior that is being changed. In this case, it would be good to add a test to make sure that GEODE-3292 has been fixed and will not reoccur. Typically I would point you towards an existing test to use as a guide, but since your changes appear to be in an area of the code that is not particularly well-tested, let me know if you don't feel confident in writing such a test and I can help. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #665: GEODE-3254: Refactoring ConfigCommands and ConfigCo...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/665#discussion_r130661310
--- Diff:
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommandDUnitTest.java
---
@@ -0,0 +1,181 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
ANY KIND, either express
+ * or implied. See the License for the specific language governing
permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.apache.commons.io.FileUtils.deleteDirectory;
+import static org.apache.geode.distributed.ConfigurationProperties.GROUPS;
+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.assertTrue;
+
+import java.io.File;
+import java.io.FileReader;
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.Properties;
+
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.internal.cache.xmlcache.CacheXmlGenerator;
+import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.internal.cli.result.CommandResult;
+import org.apache.geode.test.dunit.Host;
+import org.apache.geode.test.dunit.SerializableRunnable;
+import org.apache.geode.test.junit.categories.DistributedTest;
+import org.apache.geode.test.junit.categories.FlakyTest;
+
+@Category(DistributedTest.class)
+public class ExportConfigCommandDUnitTest extends CliCommandTestBase {
+ private File managerConfigFile;
+ private File managerPropsFile;
+ private File vm1ConfigFile;
+ private File vm1PropsFile;
+ private File vm2ConfigFile;
+ private File vm2PropsFile;
+ private File shellConfigFile;
+ private File shellPropsFile;
+ private File subDir;
+ private File subManagerConfigFile;
+
+ @Override
+ protected final void postSetUpCliCommandTestBase() throws Exception {
+this.managerConfigFile =
this.temporaryFolder.newFile("Manager-cache.xml");
+this.managerPropsFile =
this.temporaryFolder.newFile("Manager-gf.properties");
+this.vm1ConfigFile = this.temporaryFolder.newFile("VM1-cache.xml");
+this.vm1PropsFile = this.temporaryFolder.newFile("VM1-gf.properties");
+this.vm2ConfigFile = this.temporaryFolder.newFile("VM2-cache.xml");
+this.vm2PropsFile = this.temporaryFolder.newFile("VM2-gf.properties");
+this.shellConfigFile = this.temporaryFolder.newFile("Shell-cache.xml");
+this.shellPropsFile =
this.temporaryFolder.newFile("Shell-gf.properties");
+this.subDir = this.temporaryFolder.newFolder(getName());
+this.subManagerConfigFile = new File(this.subDir,
this.managerConfigFile.getName());
+ }
+
+ @Category(FlakyTest.class) // GEODE-1449
+ @Test
+ public void testExportConfig() throws Exception {
+Properties localProps = new Properties();
+localProps.setProperty(NAME, "Manager");
+localProps.setProperty(GROUPS, "Group1");
+setUpJmxManagerOnVm0ThenConnect(localProps);
+
+// Create a cache in another VM (VM1)
+Host.getHost(0).getVM(1).invoke(new SerializableRunnable() {
+ public void run() {
+Properties localProps = new Properties();
+localProps.setProperty(NAME, "VM1");
+localProps.setProperty(GROUPS, "Group2");
+getSystem(localProps);
+getCache();
+ }
+});
+
+// Create a cache in a 3rd VM (VM2)
+Host.getHost(0).getVM(2).invoke(new SerializableRunnable() {
+ public void run() {
+Properties localProps = new Properties();
+localProps.setProperty(NAME, "VM2");
+localProps.setProperty(GROUPS, "Group2");
+
[GitHub] geode pull request #665: GEODE-3254: Refactoring ConfigCommands and ConfigCo...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/665#discussion_r130658877
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/Interceptor.java
---
@@ -0,0 +1,79 @@
+/*
+ * 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.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.Map;
+
+import org.apache.geode.management.cli.Result;
+import
org.apache.geode.management.internal.cli.AbstractCliAroundInterceptor;
+import org.apache.geode.management.internal.cli.GfshParseResult;
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.geode.management.internal.cli.result.ResultBuilder;
+import org.apache.geode.management.internal.cli.shell.Gfsh;
+
+/**
+ * Interceptor used by gfsh to intercept execution of export config
command at "shell".
+ */
+public class Interceptor extends AbstractCliAroundInterceptor {
--- End diff --
This class appears to be 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 #671: GEODE-3255: Refactor CreateAlterDestroyRegionComman...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/671#discussion_r130668398
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommand.java
---
@@ -1143,4 +721,26 @@ private boolean
isAttributePersistent(RegionAttributes attributes) {
return attributes != null && attributes.getDataPolicy() != null
&& attributes.getDataPolicy().toString().contains("PERSISTENT");
}
+
+ private static boolean regionExists(InternalCache cache, String
regionPath) {
--- End diff --
I don't see any tests that validate the behavior of this method. I think
we can simplify it to:
```
private static boolean regionExists(InternalCache cache, String regionPath)
{
if (regionPath == null || Region.SEPARATOR.equals(regionPath)) {
return false;
}
ManagementService managementService =
ManagementService.getExistingManagementService(cache);
DistributedSystemMXBean dsMBean =
managementService.getDistributedSystemMXBean();
String[] allRegionPaths = dsMBean.listAllRegionPaths();
return Arrays.stream(allRegionPaths).anyMatch(regionPath::equals);
}
```
But it would be nice to have a test that would fail with this
implementation:
```
private static boolean regionExists(InternalCache cache, String regionPath)
{
return 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 #672: GEODE-3256: Refactoring DataCommands
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/672#discussion_r130673499
--- 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 tend to try to avoid `---Utils` classes, as they often end up as a bag of
unrelated methods rather than a true object-oriented class with a single
responsibility. I'm not sure where all of these methods belong in this case
though, and I think this is certainly a step in the right direction from the
old `DataCommands` monolith.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #651: GEODE-3230: Cleaning up unused (Cli)Strings
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/651#discussion_r130677047
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/process/MBeanProcessController.java
---
@@ -260,6 +265,7 @@ private JMXServiceURL getJMXServiceURL() throws
AttachNotSupportedException, IOE
// need to load the management-agent and get the address
final String javaHome =
vm.getSystemProperties().getProperty("java.home");
+assertState(StringUtils.isNotBlank(javaHome),
CliStrings.JAVA_HOME_NOT_FOUND_ERROR_MESSAGE);
--- End diff --
I worry about changing the exception the exception thrown here (before I
think it would have thrown an `IOException`).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #651: GEODE-3230: Cleaning up unused (Cli)Strings
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/651#discussion_r130677475
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/Launcher.java
---
@@ -99,7 +99,6 @@ protected Launcher() {
this.allowedCommandLineCommands.add(CliStrings.START_JCONSOLE);
this.allowedCommandLineCommands.add(CliStrings.START_JVISUALVM);
this.allowedCommandLineCommands.add(CliStrings.START_LOCATOR);
-this.allowedCommandLineCommands.add(CliStrings.START_MANAGER);
--- End diff --
Can you explain the reason this change?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #651: GEODE-3230: Cleaning up unused (Cli)Strings
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/651#discussion_r130678413
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
---
@@ -633,8 +631,8 @@ public DataCommandResult put(String key, String value,
boolean putIfAbsent, Stri
try {
keyObject = getClassObject(key, keyClass);
} catch (ClassNotFoundException e) {
-return DataCommandResult.createPutResult(key, null, null,
-"ClassNotFoundException " + keyClass, false);
+return DataCommandResult.createPutResult(key, null,
e.getException(),
--- End diff --
I think we can pass the Exception `e` itself rather than `e.getException()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #664: Fix for GEODE-3292
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/664 Thanks @jujoramos, this looks good to me. Do you have any thoughts @jinmeiliao? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #679: GEODE-3340: Refactor ConfigCommandsDUnitTest to use...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/679#discussion_r130998592
--- Diff:
geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ConfigCommandDUnitTest.java
---
@@ -0,0 +1,242 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
ANY KIND, either express
+ * or implied. See the License for the specific language governing
permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static
org.apache.geode.distributed.ConfigurationProperties.ENABLE_TIME_STATISTICS;
+import static org.apache.geode.distributed.ConfigurationProperties.GROUPS;
+import static
org.apache.geode.distributed.ConfigurationProperties.LOG_LEVEL;
+import static
org.apache.geode.distributed.ConfigurationProperties.STATISTIC_SAMPLING_ENABLED;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.distributed.internal.ClusterConfigurationService;
+import org.apache.geode.distributed.internal.DistributionConfig;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.logging.LogWriterImpl;
+import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.geode.management.internal.cli.result.CommandResult;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.test.dunit.IgnoredException;
+import org.apache.geode.test.dunit.rules.GfshShellConnectionRule;
+import org.apache.geode.test.dunit.rules.LocatorServerStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.junit.categories.DistributedTest;
+
+@Category(DistributedTest.class)
+@RunWith(JUnitParamsRunner.class)
+public class ConfigCommandDUnitTest {
+ @Rule
+ public LocatorServerStartupRule startupRule = new
LocatorServerStartupRule();
+
+ @Rule
+ public GfshShellConnectionRule gfsh = new GfshShellConnectionRule();
+
+ @Rule
+ public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+ @Test
+ @Parameters({"true", "false"})
+ public void testDescribeConfig(final boolean connectOverHttp) throws
Exception {
+Properties localProps = new Properties();
+localProps.setProperty(STATISTIC_SAMPLING_ENABLED, "true");
+localProps.setProperty(ENABLE_TIME_STATISTICS, "true");
+localProps.setProperty(GROUPS, "G1");
+MemberVM server0 = startupRule.startServerAsJmxManager(0, localProps);
+
+if (connectOverHttp) {
+ gfsh.connectAndVerify(server0.getHttpPort(),
GfshShellConnectionRule.PortType.http);
+} else {
+ gfsh.connectAndVerify(server0.getJmxPort(),
GfshShellConnectionRule.PortType.jmxManger);
+}
+
+server0.invoke(() -> {
+ InternalCache cache =
LocatorServerStartupRule.serverStarter.getCache();
+ InternalDistributedSystem system =
cache.getInternalDistributedSystem();
+ DistributionConfig config = system.getConfig();
+ config.setArchiveFileSizeLimit(1000);
+});
+
+gfsh.executeAndVerifyCommand("describe config --member=" +
server0.getName());
+String result = gfsh.getGfshOutput();
+
+assertThat(result).contains("enable-time-statistics
: true");
--- End diff --
These assertions might read more cleanly with a regex rather than a
hardcoded number of spaces, e.g.
```
assertTh
[GitHub] geode pull request #679: GEODE-3340: Refactor ConfigCommandsDUnitTest to use...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/679#discussion_r130998820
--- Diff:
geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ConfigCommandDUnitTest.java
---
@@ -0,0 +1,242 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
ANY KIND, either express
+ * or implied. See the License for the specific language governing
permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static
org.apache.geode.distributed.ConfigurationProperties.ENABLE_TIME_STATISTICS;
+import static org.apache.geode.distributed.ConfigurationProperties.GROUPS;
+import static
org.apache.geode.distributed.ConfigurationProperties.LOG_LEVEL;
+import static
org.apache.geode.distributed.ConfigurationProperties.STATISTIC_SAMPLING_ENABLED;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.distributed.internal.ClusterConfigurationService;
+import org.apache.geode.distributed.internal.DistributionConfig;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.logging.LogWriterImpl;
+import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.geode.management.internal.cli.result.CommandResult;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.test.dunit.IgnoredException;
+import org.apache.geode.test.dunit.rules.GfshShellConnectionRule;
+import org.apache.geode.test.dunit.rules.LocatorServerStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.junit.categories.DistributedTest;
+
+@Category(DistributedTest.class)
+@RunWith(JUnitParamsRunner.class)
+public class ConfigCommandDUnitTest {
--- End diff --
What was the motivation to move this test from :geode-core into :geode-web?
---
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 #668: GEODE-3306: Remove whitespace StringBuffers/nodes created ...
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/668 Hi Darren, It looks like `xercesImpl` may need to be declared as a `testCompile` dependency rather than `testRuntime` in case people are building with a JDK which does not include Xerxes. I also think it would be prudent to solicit feedback from the community via dev@geode.apache.org before we add this library, since I know that Xerces can sometimes be troublesome. (See e.g. https://stackoverflow.com/questions/11677572/dealing-with-xerces-hell-in-java-maven) Thank you, Jared Stewart --- 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 #668: GEODE-3306: Remove whitespace StringBuffers/nodes c...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/668#discussion_r131966460
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java
---
@@ -128,9 +128,16 @@ public void testCacheXmlParserWithSimplePool() {
*/
@Test
public void testCacheXmlParserWithSimplePoolXerces() {
-System.setProperty("javax.xml.parsers.SAXParserFactory",
+String prevParserFactory =
System.setProperty("javax.xml.parsers.SAXParserFactory",
--- End diff --
You can make this cleanup of the system properties more robust by using the
`RestoreSystemProperties` JUnit rule. All you need to do is add this as a
member variable of the test class:
```
@Rule
public final RestoreSystemProperties restoreSystemProperties = new
RestoreSystemProperties();
```
(For an example see
[LocatorLauncherTest](https://github.com/apache/geode/blob/d1db2f02d2ce45a437b34488934e5b1d53c7b5ca/geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherTest.java).)
---
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 #668: GEODE-3306: Remove whitespace StringBuffers/nodes c...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/668#discussion_r131967266
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParser.java
---
@@ -2596,6 +2596,18 @@ private void endDeclarable() {
public void startElement(String namespaceURI, String localName, String
qName, Attributes atts)
throws SAXException {
+// This while loop pops all StringBuffers at the top of the stack
+// that contain only whitespace; see GEODE-3306
+while (!stack.empty()) {
+ Object o = stack.peek();
+ if (o instanceof StringBuffer
+ && ((StringBuffer) o).toString().replaceAll("\\s",
"").equals("")) {
--- End diff --
I think `StringUtils.isBlank( (StringBuffer o).toString())` might be
simpler here as well as in `endElement`.
---
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 #668: GEODE-3306: Remove whitespace StringBuffers/nodes c...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/668#discussion_r131968121
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java
---
@@ -111,10 +113,31 @@ public void testCacheXmlParserWithSimplePool() {
InternalDistributedSystem system =
InternalDistributedSystem.newInstanceForTesting(dm, nonDefault);
when(dm.getSystem()).thenReturn(system);
-InternalDistributedSystem.connect(nonDefault);
-CacheXmlParser.parse(getClass()
-
.getResourceAsStream("CacheXmlParserJUnitTest.testSimpleClientCacheXml.cache.xml"));
+Cache cache = new CacheFactory()
+.set("cache-xml-file",
"CacheXmlParserJUnitTest.testSimpleClientCacheXml.cache.xml")
+.create(InternalDistributedSystem.connect(nonDefault));
+cache.close();
+ }
+
+ /**
+ * Test that {@link CacheXmlParser} can parse the test cache.xml file,
using the Apache Xerces XML
+ * parser.
+ *
+ * @since Geode 1.3
+ */
+ @Test
+ public void testCacheXmlParserWithSimplePoolXerces() {
+String prevParserFactory =
System.setProperty("javax.xml.parsers.SAXParserFactory",
+"org.apache.xerces.jaxp.SAXParserFactoryImpl");
+
+testCacheXmlParserWithSimplePool();
+
+if (prevParserFactory != null) {
--- End diff --
This cleanup of system properties can be made simpler by using the
`RestoreSystemProperties` JUnit rule. All you need to do is add this member
variable to your test class:
``` @Rule
public final RestoreSystemProperties restoreSystemProperties = new
RestoreSystemProperties();
```
(For an example, see
[LocatorLauncherTest](https://github.com/apache/geode/blob/d1db2f02d2ce45a437b34488934e5b1d53c7b5ca/geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherTest.java).)
---
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 jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132585334
--- 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 saw that you used Apache common's `Validate` class in other places.
Would that be good to use here since the default java `assert` won't do
anything under normal circumstances?
---
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 jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132562988
--- 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 --
What is the motivation for removing static here? I believe Effective Java
advocates for favoring static member classes over non-static.
---
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 jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132572256
--- 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 --
Is there any reason to include this constructor at all?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #699: GEODE-3413: overhaul launcher and process classes a...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132585045
--- 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 --
This might be a nice simplification:
```
ControlRequestHandler statusHandler = () -> {
// read the statusFile
StringBuilder lines = new StringBuilder();
try (BufferedReader reader = new BufferedReader(new
FileReader(statusFile))) {
reader.lines().forEach(lines::append);
} finally {
statusRef.set(lines.toString());
}
};
```
---
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 jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132570355
--- Diff:
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java ---
@@ -104,6 +109,8 @@
helpMap.put("bind-address",
--- End diff --
Since this class is rather large, maybe in the future we can extract the
`helpMap` and `usageMap` to a separate class like `LocatorLauncherHelp` whose
single responsibility is to provide the help and usage information.
---
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 jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132568184
--- 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 --
I believe the behavior of this method may contradicts its javadoc, which
says that
> If the argument does **not** start with '-' or is **not** the name of a
Locator launcher command, then the value is presumed to be the member name for
the Locator in GemFire.
Whereas the actual implementation seems to find values which **are** the
name of a Locator launcher command. I would guess that the javadoc is correct
about the intended behavior.
P.S. I can't resist writing this in a declarative style with streams :)
```
protected void parseMemberName(final String... args) {
if (args == null) return;
Arrays.stream(args)
.filter(arg -> !(arg.startsWith(OPTION_PREFIX) ||
Command.isCommand(arg)))
.findFirst()
.ifPresent(this::setMemberName);
}
```
---
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 jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132571788
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/config/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.internal.config;
+
+/**
+ * Exception thrown during server startup when it requests the locators
for shared configuration and
+ * does not receive it.
+ */
+public class ClusterConfigurationNotAvailableException
+extends
org.apache.geode.internal.process.ClusterConfigurationNotAvailableException {
+
+ private static final long serialVersionUID = 771319836094239284L;
--- End diff --
Since this is a new class and there is no need to worry about backwards
compatibility with an autogenerated `serialVersionUID`, I think you can just
use `0L`.
---
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 jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/699#discussion_r132569537
--- 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 --
Cool, I didn't know that `==` could be used safely for enums.
---
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 #708: GEODE-3410 Doc update for gfsh query command change...
Github user jaredjstewart commented on a diff in the pull request: https://github.com/apache/geode/pull/708#discussion_r132589415 --- Diff: geode-docs/tools_modules/gfsh/command-pages/query.html.md.erb --- @@ -21,7 +21,11 @@ limitations under the License. Run queries against Geode regions. -Run the specified OQL query as a single quoted string and displays results in pages allows to move between pages. If limit is not set in the query, then a default limit of 1000 (derived from GFSH environment variable APP\_FETCH\_SIZE) will be applied. Page size is derived from GFSH environment variable APP\_COLLECTION\_LIMIT (default value=20). +If a limit restricting the result size is not set in the query, +then a default limit of the gfsh environment variable `APP_FETCH_SIZE` --- End diff -- It might be nice to also mention that `APP_FETCH_SIZE` has a default value of 100 if not explicitly set by the user. --- 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 #708: GEODE-3410 Doc update for gfsh query command change...
Github user jaredjstewart commented on a diff in the pull request: https://github.com/apache/geode/pull/708#discussion_r132591921 --- Diff: geode-docs/tools_modules/gfsh/command-pages/query.html.md.erb --- @@ -21,7 +21,11 @@ limitations under the License. Run queries against Geode regions. -Run the specified OQL query as a single quoted string and displays results in pages allows to move between pages. If limit is not set in the query, then a default limit of 1000 (derived from GFSH environment variable APP\_FETCH\_SIZE) will be applied. Page size is derived from GFSH environment variable APP\_COLLECTION\_LIMIT (default value=20). +If a limit restricting the result size is not set in the query, +then a default limit of the gfsh environment variable `APP_FETCH_SIZE` --- End diff -- Would it be much work to add a link to that section here in that case? I just suspect if I was a user I would have no idea what "the gfsh environment variable `APP_FETCH_SIZE`" means. --- 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 #613: GEODE-3151: Internal Region Registration in JMX as per con...
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/613 Hi Dinesh, Sorry to be so slow in responding to your PR. Since the changes you are proposing in GEODE-3151 involve changing a public-facing API (in DistributionConfig, I think it first needs to be discussed on dev@geode.apache.org before we could pull this in. Would you mind sending an email to that list with your proposal and the use-case you have in mind? Thank you, Jared --- 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 #720: GEODE-3462: FunctionStats Exposed over JMX
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/720 Hi Dinesh, I just commented on your earlier PR, and the same principle applies to this one as well: Changes to public APIs should first be discussed on dev@geode.apache.org. Thank you, Jared Stewart --- 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 jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/724#discussion_r134075750
--- 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 think you meant "no see**d**"
---
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 jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/724#discussion_r134075476
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/process/lang/AvailablePidTest.java
---
@@ -100,7 +105,53 @@ public void
findAvailablePidsShouldReturnNoDuplicatedPids() throws Exception {
assertThatNoPidIsDuplicated(availablePid.findAvailablePids(8));
}
- private void assertThatNoPidIsDuplicated(int[] pids) {
+ @Test(timeout = DEFAULT_TIMEOUT_MILLIS)
+ public void findAvailablePidShouldReturnGreaterThanOrEqualToLowerBound()
throws Exception {
+availablePid = new AvailablePid(new AvailablePid.Bounds(1, 3));
+
+int pid = availablePid.findAvailablePid();
+
+assertThat(pid).isGreaterThanOrEqualTo(1);
+ }
+
+ @Test(timeout = DEFAULT_TIMEOUT_MILLIS)
+ public void findAvailablePidShouldReturnLessThanOrEqualToUpperBound()
throws Exception {
+availablePid = new AvailablePid(new AvailablePid.Bounds(1, 3));
+
+int pid = availablePid.findAvailablePid();
+
+assertThat(pid).isLessThanOrEqualTo(3);
+ }
+
+ @Test
+ public void randomLowerBoundIsInclusive() throws Exception {
+availablePid = new AvailablePid(new AvailablePid.Bounds(1, 3));
+
+await().atMost(10, SECONDS).until(() ->
assertThat(availablePid.random()).isEqualTo(1));
+ }
+
+ @Test
+ public void randomUpperBoundIsInclusive() throws Exception {
+availablePid = new AvailablePid(new AvailablePid.Bounds(1, 3));
+
+await().atMost(10, SECONDS).until(() ->
assertThat(availablePid.random()).isEqualTo(3));
--- End diff --
This is a clever way to test that lower and upper bounds are potential
output values!
---
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 jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/724#discussion_r134075704
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/process/lang/AvailablePidTest.java
---
@@ -100,7 +105,53 @@ public void
findAvailablePidsShouldReturnNoDuplicatedPids() throws Exception {
assertThatNoPidIsDuplicated(availablePid.findAvailablePids(8));
}
- private void assertThatNoPidIsDuplicated(int[] pids) {
+ @Test(timeout = DEFAULT_TIMEOUT_MILLIS)
+ public void findAvailablePidShouldReturnGreaterThanOrEqualToLowerBound()
throws Exception {
+availablePid = new AvailablePid(new AvailablePid.Bounds(1, 3));
+
+int pid = availablePid.findAvailablePid();
+
+assertThat(pid).isGreaterThanOrEqualTo(1);
--- End diff --
I wonder if trying random values for some time period (like the awaitility
tests below) might be more robust:
```
availablePid = new AvailablePid(new AvailablePid.Bounds(1, 3));
com.google.common.base.Stopwatch stopwatch = Stopwatch.createStarted();
while (stopwatch.elapsed(TimeUnit.SECONDS) < 10) {
int pid = availablePid.findAvailablePid();
assertThat(pid).isLessThanOrEqualTo(3);
}
---
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 jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/729#discussion_r134348604
--- 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 --
Did you mean to specify READER_JOIN_TIMEOUT_MILLIS instead of 2? Otherwise
it looks like that variable is only read by 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 #730: GEODE-3472: Remove a great deal of commented-out co...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/730#discussion_r134869699
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/CliUtil.java
---
@@ -212,8 +211,7 @@ public static void bytesToFiles(byte[][] fileData,
String parentDirPath, boolean
}
public static boolean isValidFileName(String filePath, String extension)
{
-boolean isValid = true;
-return isValid;
+return true;
--- End diff --
Can we just delete this method entirely?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #730: GEODE-3472: Remove a great deal of commented-out co...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/730#discussion_r134867453
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java
---
@@ -415,9 +413,8 @@ public Result toCommandResult() {
toCommandResult_isPut(section, table);
} else if (isRemove()) {
toCommandResult_isRemove(section, table);
-} else if (isSelect()) {
- // its moved to its separate method
}
+// isSelect() moved to a separate method
--- End diff --
Did you intended to leave this comment?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #730: GEODE-3472: Remove a great deal of commented-out co...
Github user jaredjstewart commented on a diff in the pull request: https://github.com/apache/geode/pull/730#discussion_r134869226 --- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java --- @@ -158,7 +156,7 @@ private Thread runner; private boolean debugON; private Terminal terminal; - private boolean supressScriptCmdOutput; + private boolean suppressScriptCadOutput; --- End diff -- This looks like a newly introduced typo :P --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #730: GEODE-3472: Remove a great deal of commented-out co...
Github user jaredjstewart commented on a diff in the pull request: https://github.com/apache/geode/pull/730#discussion_r134872863 --- Diff: geode-core/src/main/java/org/apache/geode/management/internal/MBeanJMXAdapter.java --- @@ -72,16 +72,13 @@ private DistributedMember distMember; /** - * log writer, or null if there is no distributed system available - */ - // private LogWriterI18n logger = InternalDistributedSystem.getLoggerI18n(); - - /** * public constructor */ + public static final int VALUE_NOT_AVAILABLE = -1; --- End diff -- Should this live in VMStatsMonitor since that's the only place it appears to be used? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #753: GEODE-3283: Expose parallel import and export in gf...
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/753#discussion_r136444815
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportDataCommand.java
---
@@ -41,44 +44,34 @@ public Result exportData(
@CliOption(key = CliStrings.EXPORT_DATA__REGION, mandatory = true,
optionContext = ConverterHint.REGION_PATH,
help = CliStrings.EXPORT_DATA__REGION__HELP) String regionName,
- @CliOption(key = CliStrings.EXPORT_DATA__FILE, mandatory = true,
+ @CliOption(key = CliStrings.EXPORT_DATA__FILE,
help = CliStrings.EXPORT_DATA__FILE__HELP) String filePath,
+ @CliOption(key = CliStrings.EXPORT_DATA__DIR,
+ help = CliStrings.EXPORT_DATA__DIR__HELP) String dirPath,
@CliOption(key = CliStrings.MEMBER, optionContext =
ConverterHint.MEMBERIDNAME,
- mandatory = true, help = CliStrings.EXPORT_DATA__MEMBER__HELP)
String memberNameOrId) {
+ mandatory = true, help = CliStrings.EXPORT_DATA__MEMBER__HELP)
String memberNameOrId,
+ @CliOption(key = CliStrings.EXPORT_DATA__PARALLEL,
unspecifiedDefaultValue = "false",
--- End diff --
I think it would be convenient to the user for us to add `,
specifiedDefaultValue = "true" here. That would make it so that one could
simply enter `export data --parallel` to get the behavior of `export data
--parallel=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 issue #393: GEODE-2430: Remove jar and zip files from test resources
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/393 Precheckin started --- 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 #393: GEODE-2430: Remove jar and zip files from test reso...
GitHub user jaredjstewart opened a pull request: https://github.com/apache/geode/pull/393 GEODE-2430: Remove jar and zip files from test resources You can merge this pull request into a Git repository by running: $ git pull https://github.com/jaredjstewart/geode GEODE-2430 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/393.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 #393 commit 9070ea43b93311d3c14d856663d7db3363d57264 Author: Jared Stewart Date: 2017-02-03T20:53:38Z GEODE-2430: Refactor ZipUtils commit 7ba28e3052b72d5b000e52dc350dedd4d6e36a44 Author: Jared Stewart Date: 2017-02-06T22:54:49Z GEODE-2430: Remove jar and zip files from test resources --- 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 #395: GEODE-2430: Fix failing tests
GitHub user jaredjstewart opened a pull request: https://github.com/apache/geode/pull/395 GEODE-2430: Fix failing tests You can merge this pull request into a Git repository by running: $ git pull https://github.com/jaredjstewart/geode GEODE-2430 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/395.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 #395 --- 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 #395: GEODE-2430: Fix failing tests
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/395 Precheckin started --- 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 #403: GEODE-2506 Update Spring from 4.3.2 to 4.3.6
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/403 Can you explain why the version override strategy is necessary? I think geode-pulse (or any other submodule with an explicit dependency on commons-beantuils) should end up with version 1.9.3 already without any change to `dependency-resolution.gradle`, since the version you've specified (1.9.3) is higher than the one coming in transitively (1.8.3). --- 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 #403: GEODE-2506 Update Spring from 4.3.2 to 4.3.6
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/403 It seems like it might be simpler to add an explicit dependency for commons-beanutils to geode-core. That way if we later introduce some new dependency which requires say commons-beanutils 1.10.1, there will be no need to troubleshoot since dependency resolution will still work as expected. --- 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 #417: GEODE-2621: Reduce time sensitivity of ExportLogsDU...
GitHub user jaredjstewart opened a pull request: https://github.com/apache/geode/pull/417 GEODE-2621: Reduce time sensitivity of ExportLogsDUnitTest Precheckin started (still running) You can merge this pull request into a Git repository by running: $ git pull https://github.com/jaredjstewart/geode GEODE-2621 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/417.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 #417 commit 55564a300571bf4bc50c34527480beaf3451c38d Author: Jared Stewart Date: 2017-03-07T23:26:20Z GEODE-2621: Reduce time sensitivity of ExportLogsDUnitTest --- 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 #417: GEODE-2621: Reduce time sensitivity of ExportLogsDUnitTest
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/417 Weâll see if the nightly build passes in the morning :D > On Mar 7, 2017, at 4:07 PM, Kenneth Howe wrote: > > @pdxrunner commented on this pull request. > > Can you remove the Flaky annotation on the test now? > > â > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub <https://github.com/apache/geode/pull/417#pullrequestreview-25667153>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF8IKWTmBCvAWNiSGDoJVLMowL7K1s0Fks5rjfEsgaJpZM4MWK0->. > --- 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 #428: GEODE-2649: Export logs does not use file creation ...
GitHub user jaredjstewart opened a pull request: https://github.com/apache/geode/pull/428 GEODE-2649: Export logs does not use file creation time Precheckin started (still running) You can merge this pull request into a Git repository by running: $ git pull https://github.com/jaredjstewart/geode GEODE-2649 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/428.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 #428 commit d53af077f8a8fbe84ff500c6f6deb2020c98f0b5 Author: Jared Stewart Date: 2017-03-16T18:12:50Z GEODE-2649: Export logs does not use file creation time --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #428: GEODE-2649: Export logs does not use file creation ...
Github user jaredjstewart closed the pull request at: https://github.com/apache/geode/pull/428 --- 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 #428: GEODE-2649: Export logs does not use file creation time
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/428 Merged as 400552a --- 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 #429: Geode-2686
GitHub user jaredjstewart opened a pull request: https://github.com/apache/geode/pull/429 Geode-2686 GEODE-2686: Remove JarClassLoader ⦠- Remove JarClassLoader - Replace ClassPathLoader's collection of JarClassLoaders with a single URLClassLoader - Change naming scheme for deployed jars from 'vf.gf#myJar.jar#1' to 'myJar.v1.jar' Change singleton implementation of ClassPathLoader from AtomicReference to volatile JarDeployer is threadsafe You can merge this pull request into a Git repository by running: $ git pull https://github.com/jaredjstewart/geode GEODE-2686 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/429.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 #429 commit 7788dba702a58e2c3997b974badd8e45e861b47f Author: Jared Stewart Date: 2017-01-19T20:00:04Z GEODE-2686: Remove JarClassLoader - Remove JarClassLoader - Replace ClassPathLoader's collection of JarClassLoaders with a single URLClassLoader - Change naming scheme for deployed jars from 'vf.gf#myJar.jar#1' to 'myJar.v1.jar' commit b9e511262b853e6f7ae346eb414138813c0e7a2d Author: Jared Stewart Date: 2017-03-17T00:22:59Z Change singleton implementation of ClassPathLoader from AtomicReference to volatile commit 5d50b12cc389293246545f08608663df9f962419 Author: Jared Stewart Date: 2017-03-17T20:59:32Z JarDeployer is threadsafe --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #429: Geode-2686
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/429 Precheckin is started (still running) --- 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 #431: GEODE-2692: Fix tests relying on file creation time
GitHub user jaredjstewart opened a pull request: https://github.com/apache/geode/pull/431 GEODE-2692: Fix tests relying on file creation time You can merge this pull request into a Git repository by running: $ git pull https://github.com/jaredjstewart/geode GEODE-2692 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/431.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 #431 commit 84a1a9bf7458df61461f07b2dd88a3eb81de94d2 Author: Jared Stewart Date: 2017-03-20T16:47:20Z GEODE-2692: Fix tests relying on file creation time --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #429: Geode-2686
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/429#discussion_r107192442
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
---
@@ -67,18 +68,16 @@ public static void
deployJarsReceivedFromClusterConfiguration(Cache cache,
String[] jarFileNames = response.getJarNames();
byte[][] jarBytes = response.getJars();
-final JarDeployer jarDeployer = new JarDeployer(
-((GemFireCacheImpl)
cache).getDistributedSystem().getConfig().getDeployWorkingDir());
-
/**
* Un-deploy the existing jars, deployed during cache creation, do not
delete anything
*/
if (jarFileNames != null && jarBytes != null) {
- JarClassLoader[] jarClassLoaders = jarDeployer.deploy(jarFileNames,
jarBytes);
+ List jarClassLoaders =
+
ClassPathLoader.getLatest().getJarDeployer().deploy(jarFileNames, jarBytes);
for (int i = 0; i < jarFileNames.length; i++) {
-if (jarClassLoaders[i] != null) {
- logger.info("Deployed " +
(jarClassLoaders[i].getFileCanonicalPath()));
+if (jarClassLoaders.get(i) != null) {
--- End diff --
This loop is just printing the jars that got deployed from
ClusterConfiguration on member startup. If another thread deploys a jar in the
middle of this loop, this list won't be updated to include that jar. But I
believe that is the desired behavior.
---
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 #429: Geode-2686
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/429#discussion_r107192575
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java
---
@@ -39,83 +44,63 @@
@Rule
public TemporaryFolder temporaryFolder = new TemporaryFolder();
- @Rule
- public RestoreSystemProperties restoreSystemProperties = new
RestoreSystemProperties();
+ JarDeployer jarDeployer;
@Before
public void setup() {
-System.setProperty("user.dir",
temporaryFolder.getRoot().getAbsolutePath());
classBuilder = new ClassBuilder();
-ClassPathLoader.setLatestToDefault();
+jarDeployer = new JarDeployer(temporaryFolder.getRoot());
}
- @Test
- public void testDeployFileAndChange() throws Exception {
-final JarDeployer jarDeployer = new JarDeployer();
+ private byte[] createJarWithClass(String className) throws IOException {
+String stringBuilder = "package integration.parent;" + "public class "
+ className + " {}";
-// First deploy of the JAR file
-byte[] jarBytes = this.classBuilder.createJarFromName("ClassA");
-JarClassLoader jarClassLoader =
-jarDeployer.deploy(new String[] {"JarDeployerDUnit.jar"}, new
byte[][] {jarBytes})[0];
-File deployedJar = new File(jarClassLoader.getFileCanonicalPath());
+return
this.classBuilder.createJarFromClassContent("integration/parent/" + className,
+stringBuilder);
+ }
-assertThat(deployedJar).exists();
-assertThat(deployedJar.getName()).contains("#1");
-assertThat(deployedJar.getName()).doesNotContain("#2");
+ @Test
+ public void testFileVersioning() throws IOException,
ClassNotFoundException {
+String jarName = "JarDeployerIntegrationTest.jar";
-assertThat(ClassPathLoader.getLatest().forName("ClassA")).isNotNull();
+byte[] firstJarBytes = createJarWithClass("ClassA");
-assertThat(doesFileMatchBytes(deployedJar, jarBytes));
+// First deploy of the JAR file
+DeployedJar firstDeployedJar =
jarDeployer.deployWithoutRegistering(jarName, firstJarBytes);
-// Now deploy an updated JAR file and make sure that the next version
of the JAR file
-// was created and the first one was deleted.
-jarBytes = this.classBuilder.createJarFromName("ClassB");
-JarClassLoader newJarClassLoader =
-jarDeployer.deploy(new String[] {"JarDeployerDUnit.jar"}, new
byte[][] {jarBytes})[0];
-File nextDeployedJar = new
File(newJarClassLoader.getFileCanonicalPath());
+
assertThat(firstDeployedJar.getFile()).exists().hasBinaryContent(firstJarBytes);
+
assertThat(firstDeployedJar.getFile().getName()).contains(".v1.").doesNotContain(".v2.");
-assertThat(nextDeployedJar.exists());
-assertThat(nextDeployedJar.getName()).contains("#2");
-assertThat(doesFileMatchBytes(nextDeployedJar, jarBytes));
+// Now deploy an updated JAR file and make sure that the next version
of the JAR file
+// was created
+byte[] secondJarBytes = createJarWithClass("ClassB");
-assertThat(ClassPathLoader.getLatest().forName("ClassB")).isNotNull();
+DeployedJar secondDeployedJar =
jarDeployer.deployWithoutRegistering(jarName, secondJarBytes);
+File secondDeployedJarFile = new
File(secondDeployedJar.getFileCanonicalPath());
-assertThatThrownBy(() -> ClassPathLoader.getLatest().forName("ClassA"))
-.isExactlyInstanceOf(ClassNotFoundException.class);
+
assertThat(secondDeployedJarFile).exists().hasBinaryContent(secondJarBytes);
+
assertThat(secondDeployedJarFile.getName()).contains(".v2.").doesNotContain(".v1.");
-
assertThat(jarDeployer.findSortedOldVersionsOfJar("JarDeployerDUnit.jar")).hasSize(1);
+File[] sortedOldJars = jarDeployer.findSortedOldVersionsOfJar(jarName);
+assertThat(sortedOldJars).hasSize(2);
+assertThat(sortedOldJars[0].getName()).contains(".v2.");
+assertThat(sortedOldJars[1].getName()).contains(".v1.");
assertThat(jarDeployer.findDistinctDeployedJars()).hasSize(1);
}
@Test
- public void testDeployNoUpdateWhenNoChange() throws Exception {
-final JarDeployer jarDeployer = new JarDeployer();
-
-// First deploy of the JAR file
-byte[] jarBytes =
this.classBuilder.createJarFromName("JarDeployerDUnitDNUWNC");
-JarClassLoader jarClassLoader =
-jarDepl
[GitHub] geode pull request #429: Geode-2686
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/429#discussion_r107194282
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java
---
@@ -126,14 +111,16 @@ public void testDeployToInvalidDirectory() throws
IOException, ClassNotFoundExce
public void run() {
try {
barrier.await();
-} catch (Exception e) {
- fail(e);
+} catch (InterruptedException iex) {
--- End diff --
This wasn't a test I wrote, it's been around for awhile. I pulled it out
of a DUnit into this class in 917dfa0. It wasn't actually clear to me what the
author was trying to test here, so I left it around intending to come back to
it later. Do you have any idea of what the author was trying to test? The
name of the test (testDeployToInvalidDirectory) seems unrelated to Concurrency.
---
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 #429: Geode-2686
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/429#discussion_r107195612
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/ClassPathLoaderTest.java ---
@@ -65,6 +65,26 @@ public void testLatestExists() throws Exception {
assertNotNull(ClassPathLoader.getLatest());
}
+ @Test
+ public void testZeroLengthFile() throws IOException,
ClassNotFoundException {
+try {
+ ClassPathLoader.getLatest().getJarDeployer().deploy(new String[]
{"JarDeployerDUnitZLF.jar"},
+ new byte[][] {new byte[0]});
+ fail("Zero length files are not deployable");
+} catch (IllegalArgumentException expected) {
--- End diff --
I'll clean this up.
---
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 #429: Geode-2686
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/429#discussion_r107195082
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java
---
@@ -144,40 +131,99 @@ public void run() {
Thread.sleep(500);
alternateDir.mkdir();
thread.join();
-} catch (Exception e) {
- fail(e);
+} catch (InterruptedException iex) {
+ fail("Interrupted while waiting.");
--- End diff --
I made sure to avoid this antipattern in the tests I added, but I had left
some old tests around unchanged. I'll go back and clean them up today.
---
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 #429: Geode-2686
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/geode/pull/429#discussion_r107221701
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java
---
@@ -126,14 +111,16 @@ public void testDeployToInvalidDirectory() throws
IOException, ClassNotFoundExce
public void run() {
try {
barrier.await();
-} catch (Exception e) {
- fail(e);
+} catch (InterruptedException iex) {
--- End diff --
Do you have an idea for how to avoid this line?
```
Thread.sleep(500) you have commented with `//don't ever do this
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #429: Geode-2686
Github user jaredjstewart closed the pull request at: https://github.com/apache/geode/pull/429 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #429: Geode-2686
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/429 Closing this PR until GEODE-2705 and a few other concerns are addressed --- 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] incubator-geode pull request #287: GEODE-2112: Fix UITests failing due to in...
GitHub user jaredjstewart opened a pull request: https://github.com/apache/incubator-geode/pull/287 GEODE-2112: Fix UITests failing due to insufficient cleanup - Extracted setup of the Server and WebDriver for Pulse tests into JUnit Rules - Refactored UITests - Removed a lot of unused test code You can merge this pull request into a Git repository by running: $ git pull https://github.com/jaredjstewart/incubator-geode feature/GEODE-2112-2 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-geode/pull/287.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 #287 commit 124dacfab1e48e3a32eae867c77e9e4733aab096 Author: Jared Stewart Date: 2016-11-17T22:59:20Z GEODE-2112: Fix UITests failing due to insufficient cleanup - Extracted setup of the Server and WebDriver for Pulse tests into JUnit Rules - Refactored UITests - Removed a lot of unused test code --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] incubator-geode issue #287: GEODE-2112: Fix UITests failing due to insuffici...
Github user jaredjstewart commented on the issue: https://github.com/apache/incubator-geode/pull/287 Precheckin started --- 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] incubator-geode pull request #289: GEODE-2117: Pulse handles float type mbea...
GitHub user jaredjstewart opened a pull request: https://github.com/apache/incubator-geode/pull/289 GEODE-2117: Pulse handles float type mbean attributes - Added tests for JMXDataUpdater::getDoubleAttribute() - JMXDataUpdater::getDoubleAttribute() now returns double approximations for floats rather than logging an error and returning zero. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jaredjstewart/incubator-geode feature/GEODE-2117 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-geode/pull/289.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 #289 commit 36929fe5282339a7c0ac4aa4dbc5ee529524b642 Author: Jared Stewart Date: 2016-11-16T23:22:34Z GEODE-2117: Pulse handles float type mbean attributes - Added tests for JMXDataUpdater::getDoubleAttribute() - JMXDataUpdater::getDoubleAttribute() now returns double approximations for floats rather than logging an error and returning zero. --- 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] incubator-geode issue #289: GEODE-2117: Pulse handles float type mbean attri...
Github user jaredjstewart commented on the issue: https://github.com/apache/incubator-geode/pull/289 Precheckin started --- 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] incubator-geode issue #289: GEODE-2117: Pulse handles float type mbean attri...
Github user jaredjstewart commented on the issue: https://github.com/apache/incubator-geode/pull/289 Precheckin passed except for a FlakyTest: ``` org.apache.geode.management.internal.cli.commands.IndexCommandsDUnitTest > testCreateDestroyUpdatesSharedConfig FAILED org.apache.geode.test.dunit.RMIException: While invoking org.apache.geode.management.internal.cli.commands.IndexCommandsDUnitTest$4.run in VM 1 running on Host 7f51a3c2-f764-4e03-5bec-1821c26ca879 with 4 VMs at org.apache.geode.test.dunit.VM.invoke(VM.java:344) at org.apache.geode.test.dunit.VM.invoke(VM.java:314) at org.apache.geode.test.dunit.VM.invoke(VM.java:259) at org.apache.geode.management.internal.cli.commands.IndexCommandsDUnitTest.testCreateDestroyUpdatesSharedConfig(IndexCommandsDUnitTest.java:686) Caused by: java.lang.AssertionError ``` --- 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] incubator-geode pull request #294: Properly close WebDriver for UITests
GitHub user jaredjstewart opened a pull request: https://github.com/apache/incubator-geode/pull/294 Properly close WebDriver for UITests - WebDriver gets closed properly so that the UITests don't overwhelm CI machines with extra processes - Increase WebDriver element wait time to reduce test flakiness You can merge this pull request into a Git repository by running: $ git pull https://github.com/jaredjstewart/incubator-geode closeWebDriverProperlyForUITests Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-geode/pull/294.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 #294 commit e9c15a3bcdef571e7e039cd751c063ede3c60b45 Author: Jared Stewart Date: 2016-11-22T18:52:52Z Properly close WebDriver for UITests - WebDriver gets closed properly so that the UITests don't overwhelm CI machines with extra processes - Increase WebDriver element wait time to reduce test flakiness --- 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] incubator-geode issue #294: Properly close WebDriver for UITests
Github user jaredjstewart commented on the issue: https://github.com/apache/incubator-geode/pull/294 Precheckin started --- 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] incubator-geode pull request #294: Properly close WebDriver for UITests
Github user jaredjstewart commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/294#discussion_r90124555
--- Diff:
geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/rules/ScreenshotOnFailureRule.java
---
@@ -45,7 +45,9 @@ private void takeScreenshot(String screenshotName) {
if (driver instanceof TakesScreenshot) {
File tempFile = ((TakesScreenshot)
driver).getScreenshotAs(OutputType.FILE);
try {
-FileUtils.copyFile(tempFile, new File("build/screenshots/" +
screenshotName + ".png"));
+File screenshot = new File("build/screenshots/" + screenshotName +
".png");
+FileUtils.copyFile(tempFile, screenshot);
+System.err.println("Screenshot saved to: " +
screenshot.getCanonicalPath());
--- End diff --
This is intentional, it adds some output to help find where the screenshots
end up when one gets taken.
---
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] incubator-geode pull request #301: UITests actually take screenshots on fail...
GitHub user jaredjstewart opened a pull request: https://github.com/apache/incubator-geode/pull/301 UITests actually take screenshots on failure The ScreenShotOnFailureRule had been moved inside of another Rule, but this doesn't actually work. So I moved it back out into each UI test separately. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jaredjstewart/incubator-geode fixScreenshotRule Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-geode/pull/301.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 #301 --- 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 #301: UITests actually take screenshots on failure
Github user jaredjstewart commented on the issue: https://github.com/apache/geode/pull/301 Precheckin passed --- 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 #309: [GEODE-2196] Add test for Cluster Config. Refactor ...
GitHub user jaredjstewart opened a pull request: https://github.com/apache/geode/pull/309 [GEODE-2196] Add test for Cluster Config. Refactor LocatorServerStart⦠â¦upRule. - Add ClusterConfigDUnitTest - Refactor LocatorServerStartupRule You can merge this pull request into a Git repository by running: $ git pull https://github.com/jaredjstewart/geode feature/GEODE-2196 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/309.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 #309 commit de203cd386ab57348303eb5938d52d4521da7fc6 Author: Jared Stewart Date: 2016-12-08T23:03:54Z [GEODE-2196] Add test for Cluster Config. Refactor LocatorServerStartupRule. - Add ClusterConfigDUnitTest - Refactor LocatorServerStartupRule --- 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 #310: [GEODE-2144] Improve error message when no security...
GitHub user jaredjstewart opened a pull request: https://github.com/apache/geode/pull/310 [GEODE-2144] Improve error message when no security credentials are p⦠â¦rovided. - Improve error message when no security credentials are provided. - Add tests for the code paths exposing this message. - Refactor making those code paths more testable. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jaredjstewart/geode feature/GEODE-2144 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/310.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 #310 commit a4538edcc17693ad7d37d9e35b5ae1d267a796e5 Author: Jared Stewart Date: 2016-12-09T19:50:00Z [GEODE-2144] Improve error message when no security credentials are provided. - Improve error message when no security credentials are provided. - Add tests for the code paths exposing this message. - Refactor making those code paths more testable. --- 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. ---
