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/07/24 18:48:13 UTC
[GitHub] geode pull request #651: GEODE-3230: Cleaning up unused (Cli)Strings
GitHub user YehEmily opened a pull request:
https://github.com/apache/geode/pull/651
GEODE-3230: Cleaning up unused (Cli)Strings
[View the JIRA ticket here.](https://issues.apache.org/jira/browse/GEODE-3230)
`CliStrings.java` contained a lot of Strings that were not used anywhere in the code. In this PR, each of these seemingly unused Strings were examined to find potential uses of them in the code. Strings that did seem useful were inserted into the code as needed. Strings that didn't seem to be useful were deleted. Strings that don't seem to be used, but are used in the closed side, were preserved and tagged as such.
(Note: An attempt was made to use all Strings that weren't currently being used in parts of the code that seemed appropriate, but most of these were removed because they broke tests.)
**Testing Status**: **Precheckin running**
### For all changes:
- [x] JIRA ticket
- [x] PR rebased
- [x] Initial commit is squashed and single
- [x] `gradlew build` runs cleanly
- [x] Tests updated to verify changes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/YehEmily/geode GEODE-3230
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode/pull/651.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 #651
----
commit 06dcd340a33c83cab7b4700a3c56e8fe780d0964
Author: YehEmily <em...@gmail.com>
Date: 2017-07-21T21:29:55Z
GEODE-3230: Cleaning up unused (Cli)Strings
----
---
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 #651: GEODE-3230: Cleaning up unused (Cli)Strings
Posted by jaredjstewart <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #651: GEODE-3230: Cleaning up unused (Cli)Strings
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily commented on a diff in the pull request:
https://github.com/apache/geode/pull/651#discussion_r130958981
--- 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 --
You're right... I was trying to find a place for the `JAVA_HOME_NOT_FOUND_ERROR_MESSAGE` variable, but it looks like it would change some behavior if inserted here. I'll just roll back the changes on this file and delete `JAVA_HOME_NOT_FOUND_ERROR_MESSAGE`. Thanks for your feedback!
---
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 #651: GEODE-3230: Cleaning up unused (Cli)Strings
Posted by jaredjstewart <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #651: GEODE-3230: Cleaning up unused (Cli)Strings
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/geode/pull/651
---
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 #651: GEODE-3230: Cleaning up unused (Cli)Strings
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily commented on a diff in the pull request:
https://github.com/apache/geode/pull/651#discussion_r129654481
--- 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 --
Good catch! I fixed this and updated the PR. 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 #651: GEODE-3230: Cleaning up unused (Cli)Strings
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily commented on a diff in the pull request:
https://github.com/apache/geode/pull/651#discussion_r130959557
--- 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 --
Sure! Almost none of the `--- manager` CliStrings were used, so I deleted them. Only `start manager` was ever used, and only in this place, so I figured it would be safe to get rid of it. Do you think this might break any backwards compatibility / should I roll back the 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #651: GEODE-3230: Cleaning up unused (Cli)Strings
Posted by jaredjstewart <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #651: GEODE-3230: Cleaning up unused (Cli)Strings
Posted by jaredjstewart <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #651: GEODE-3230: Cleaning up unused (Cli)Strings
Posted by YehEmily <gi...@git.apache.org>.
Github user YehEmily commented on a diff in the pull request:
https://github.com/apache/geode/pull/651#discussion_r130959623
--- 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 --
Fixed - thanks for your feedback!
---
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 #651: GEODE-3230: Cleaning up unused (Cli)Strings
Posted by pdxrunner <gi...@git.apache.org>.
Github user pdxrunner commented on a diff in the pull request:
https://github.com/apache/geode/pull/651#discussion_r130982335
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java ---
@@ -633,8 +631,8 @@ public DataCommandResult put(String key, String value, boolean putIfAbsent, Stri
try {
keyObject = getClassObject(key, keyClass);
} catch (ClassNotFoundException e) {
- return DataCommandResult.createPutResult(key, null, null,
- "ClassNotFoundException " + keyClass, false);
+ return DataCommandResult.createPutResult(key, null, e.getException(),
--- End diff --
I don't understand the context of this change - the message was "ClassNotFound" but now it's "key not found" with a key class?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---