You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by YehEmily <gi...@git.apache.org> on 2017/05/30 20:27:39 UTC
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
GitHub user YehEmily opened a pull request:
https://github.com/apache/geode/pull/549
GEODE-2203: gfsh status locator/server - Command now gives more descr…
…iptive output on empty parameter
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/YehEmily/geode GEODE-2203
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode/pull/549.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 #549
----
commit 784ded407a0d16dc7fbf3c6693d0e1a8443fdd44
Author: YehEmily <em...@gmail.com>
Date: 2017-05-26T22:54:46Z
GEODE-2203: gfsh status locator/server - Command now gives more descriptive output on empty parameter
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily closed the pull request at:
https://github.com/apache/geode/pull/549
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Posted by jaredjstewart <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Posted by pdxrunner <gi...@git.apache.org>.
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/549#discussion_r119419356
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java ---
@@ -771,11 +771,19 @@ public Result statusLocator(
CliStrings.STATUS_SERVICE__GFSH_NOT_CONNECTED_ERROR_MESSAGE, LOCATOR_TERM_NAME));
}
} else {
- final LocatorLauncher locatorLauncher =
- new LocatorLauncher.Builder().setCommand(LocatorLauncher.Command.STATUS)
- .setBindAddress(locatorHost).setDebug(isDebugging()).setPid(pid)
- .setPort(locatorPort).setWorkingDirectory(workingDirectory).build();
-
+ final LocatorLauncher locatorLauncher;
+
+ if ((locatorHost == null) && (locatorPort == null) && (workingDirectory == null)
+ && (pid == null)) {
+ return ResultBuilder.createShellClientErrorResult(
+ String.format(CliStrings.STATUS_LOCATOR__NO_LOCATOR_SPECIFIED_ERROR_MESSAGE,
+ getLocatorId(locatorHost, locatorPort),
+ StringUtils.defaultIfBlank(workingDirectory, SystemUtils.CURRENT_DIRECTORY)));
--- End diff --
I thought that the simplification of this argument (don't need the defaultIfBlank call) was in a previous commit, but it seems to have been reverted in your latest commit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Posted by jaredjstewart <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Posted by jaredjstewart <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily commented on a diff in the pull request:
https://github.com/apache/geode/pull/549#discussion_r119225590
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java ---
@@ -771,11 +771,18 @@ public Result statusLocator(
CliStrings.STATUS_SERVICE__GFSH_NOT_CONNECTED_ERROR_MESSAGE, LOCATOR_TERM_NAME));
}
} else {
- final LocatorLauncher locatorLauncher =
- new LocatorLauncher.Builder().setCommand(LocatorLauncher.Command.STATUS)
- .setBindAddress(locatorHost).setDebug(isDebugging()).setPid(pid)
- .setPort(locatorPort).setWorkingDirectory(workingDirectory).build();
+ final LocatorLauncher locatorLauncher;
+ if ((locatorHost == null) && (locatorPort == null) && (workingDirectory == null)) {
+ return ResultBuilder.createShellClientErrorResult(
+ String.format(CliStrings.STATUS_LOCATOR__NO_LOCATOR_SPECIFIED_ERROR_MESSAGE,
+ getLocatorId(locatorHost, locatorPort),
+ StringUtils.defaultIfBlank(workingDirectory, SystemUtils.CURRENT_DIRECTORY)));
--- End diff --
Got it - thanks! I'll update the PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily commented on a diff in the pull request:
https://github.com/apache/geode/pull/549#discussion_r119225532
--- 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 think that if a value is given for locatorPort but not for locatorHost, `locatorLauncher` will just have a null value for locatorPort, with no errors (in the `else` statement). Do you mean there will be an error in a different part of the 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #549: GEODE-2203: gfsh status locator/server - Command now gives...
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily commented on the issue:
https://github.com/apache/geode/pull/549
Precheckin pending!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily commented on a diff in the pull request:
https://github.com/apache/geode/pull/549#discussion_r119422035
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java ---
@@ -771,11 +771,19 @@ public Result statusLocator(
CliStrings.STATUS_SERVICE__GFSH_NOT_CONNECTED_ERROR_MESSAGE, LOCATOR_TERM_NAME));
}
} else {
- final LocatorLauncher locatorLauncher =
- new LocatorLauncher.Builder().setCommand(LocatorLauncher.Command.STATUS)
- .setBindAddress(locatorHost).setDebug(isDebugging()).setPid(pid)
- .setPort(locatorPort).setWorkingDirectory(workingDirectory).build();
-
+ final LocatorLauncher locatorLauncher;
+
+ if ((locatorHost == null) && (locatorPort == null) && (workingDirectory == null)
+ && (pid == null)) {
+ return ResultBuilder.createShellClientErrorResult(
+ String.format(CliStrings.STATUS_LOCATOR__NO_LOCATOR_SPECIFIED_ERROR_MESSAGE,
+ getLocatorId(locatorHost, locatorPort),
+ StringUtils.defaultIfBlank(workingDirectory, SystemUtils.CURRENT_DIRECTORY)));
--- End diff --
Oops! Sorry, I'll fix that right away - thank you!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Posted by pdxrunner <gi...@git.apache.org>.
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/549#discussion_r119221757
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java ---
@@ -771,11 +771,18 @@ public Result statusLocator(
CliStrings.STATUS_SERVICE__GFSH_NOT_CONNECTED_ERROR_MESSAGE, LOCATOR_TERM_NAME));
}
} else {
- final LocatorLauncher locatorLauncher =
- new LocatorLauncher.Builder().setCommand(LocatorLauncher.Command.STATUS)
- .setBindAddress(locatorHost).setDebug(isDebugging()).setPid(pid)
- .setPort(locatorPort).setWorkingDirectory(workingDirectory).build();
+ final LocatorLauncher locatorLauncher;
+ if ((locatorHost == null) && (locatorPort == null) && (workingDirectory == null)) {
+ return ResultBuilder.createShellClientErrorResult(
+ String.format(CliStrings.STATUS_LOCATOR__NO_LOCATOR_SPECIFIED_ERROR_MESSAGE,
+ getLocatorId(locatorHost, locatorPort),
+ StringUtils.defaultIfBlank(workingDirectory, SystemUtils.CURRENT_DIRECTORY)));
--- End diff --
workingDirectory == null in this clause of the if, so this argument can just be SystemUtils.CURRENT_DIRECTORY
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily commented on a diff in the pull request:
https://github.com/apache/geode/pull/549#discussion_r119225797
--- 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 --
Oops, that's very true - thanks! I'll update the PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Posted by jaredjstewart <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Posted by jaredjstewart <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---