[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
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 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 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/549#discussion_r119419356
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
---
@@ -771,11 +771,19 @@ public Result statusLocator(
CliStrings.STATUS_SERVICE__GFSH_NOT_CONNECTED_ERROR_MESSAGE,
LOCATOR_TERM_NAME));
}
} else {
-final LocatorLauncher locatorLauncher =
-new
LocatorLauncher.Builder().setCommand(LocatorLauncher.Command.STATUS)
-
.setBindAddress(locatorHost).setDebug(isDebugging()).setPid(pid)
-
.setPort(locatorPort).setWorkingDirectory(workingDirectory).build();
-
+final LocatorLauncher locatorLauncher;
+
+if ((locatorHost == null) && (locatorPort == null) &&
(workingDirectory == null)
+&& (pid == null)) {
+ return ResultBuilder.createShellClientErrorResult(
+
String.format(CliStrings.STATUS_LOCATOR__NO_LOCATOR_SPECIFIED_ERROR_MESSAGE,
+ getLocatorId(locatorHost, locatorPort),
+ StringUtils.defaultIfBlank(workingDirectory,
SystemUtils.CURRENT_DIRECTORY)));
--- End diff --
I thought that the simplification of this argument (don't need the
defaultIfBlank call) was in a previous commit, but it seems to have been
reverted in your latest commit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Github user 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 #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_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 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 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 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 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 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #549: GEODE-2203: gfsh status locator/server - Command no...
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/549#discussion_r119221757
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
---
@@ -771,11 +771,18 @@ public Result statusLocator(
CliStrings.STATUS_SERVICE__GFSH_NOT_CONNECTED_ERROR_MESSAGE,
LOCATOR_TERM_NAME));
}
} else {
-final LocatorLauncher locatorLauncher =
-new
LocatorLauncher.Builder().setCommand(LocatorLauncher.Command.STATUS)
-
.setBindAddress(locatorHost).setDebug(isDebugging()).setPid(pid)
-
.setPort(locatorPort).setWorkingDirectory(workingDirectory).build();
+final LocatorLauncher locatorLauncher;
+if ((locatorHost == null) && (locatorPort == null) &&
(workingDirectory == null)) {
+ return ResultBuilder.createShellClientErrorResult(
+
String.format(CliStrings.STATUS_LOCATOR__NO_LOCATOR_SPECIFIED_ERROR_MESSAGE,
+ getLocatorId(locatorHost, locatorPort),
+ StringUtils.defaultIfBlank(workingDirectory,
SystemUtils.CURRENT_DIRECTORY)));
--- End diff --
workingDirectory == null in this clause of the if, so this argument can
just be SystemUtils.CURRENT_DIRECTORY
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[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_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 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 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
