----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62163/#review184843 -----------------------------------------------------------
Overall: I love how much is getting pruned! I like to run inspections with the `Changed Files` scope to clean up some of the low-hanging fruit. In particular, I get rankled by * Missorted modifiers (`ExportLogsCommand`) * Unnecessary fully qualified names (a few that could be replace by imports) * Unnecessary interface modifies (`CliMetaData` and `CommandStatement`) * Collapsable catch blocks * Explicit type can be reoplace with `<>` * Unnecessary semicolons It's the sort of stuff that really doesn't matter, but is easy to touch up when you're in the file anyway. geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java Lines 110 (patched) <https://reviews.apache.org/r/62163/#comment261033> Does Java respect the standard macro classes? `[^\s]` can be the more succinct `\S`. geode-core/src/main/java/org/apache/geode/management/cli/CommandStatement.java Lines 44-51 (original) <https://reviews.apache.org/r/62163/#comment261036> This is a non-internal interface. Would it not be better to leave it as-is until we can remove the interface altogether in Geode 1.4+? geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java Lines 254-259 (original), 250-255 (patched) <https://reviews.apache.org/r/62163/#comment261038> Not entirely clear what's going on here, but `String query` is dead now and can be removed. geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommand.java Line 132 (original), 119-124 (patched) <https://reviews.apache.org/r/62163/#comment261042> Since part of this rework is allowing commands to throw and be caught by the executor, is that something we'd like / would be able to do here as well? I feel like it makes more sence to throw these errors rather than return an `ErrorResult`, if that's possible. geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportImportClusterConfigurationCommands.java Line 110 (original), 116 (patched) <https://reviews.apache.org/r/62163/#comment261043> Should we just rethrow the exception, now that we can handle them on the executor? geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandStatementImpl.java Lines 44-48 (original), 42-46 (patched) <https://reviews.apache.org/r/62163/#comment261037> Kill the dead code. geode-core/src/main/java/org/apache/geode/management/internal/cli/result/DownloadFileResult.java Lines 74-82 (patched) <https://reviews.apache.org/r/62163/#comment261041> Remove dead code. geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ShellCommandsController.java Line 289 (original), 180 (patched) <https://reviews.apache.org/r/62163/#comment261046> Spelling. geode-core/src/main/java/org/apache/geode/management/internal/web/util/ConvertUtils.java Line 55 (original), 56-59 (patched) <https://reviews.apache.org/r/62163/#comment261039> Early exits are so much better than extra nesting. Hurrah! geode-core/src/test/java/org/apache/geode/internal/util/ArgumentRedactorJUnitTest.java Lines 129 (patched) <https://reviews.apache.org/r/62163/#comment261034> Spelling. geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployWithGroupsDUnitTest.java Lines 104 (patched) <https://reviews.apache.org/r/62163/#comment261040> Remove dead code. geode-core/src/test/java/org/apache/geode/management/internal/web/http/ClientHttpRequestTest.java Line 25 (original), 21 (patched) <https://reviews.apache.org/r/62163/#comment261045> This class was introduced but is unused? geode-junit/build.gradle Lines 26 (patched) <https://reviews.apache.org/r/62163/#comment261035> Do we need to start a thread on geode-dev about adding dependencies? Or is the JUnit module somewhat outside that? - Patrick Rhomberg On Sept. 7, 2017, 3:33 p.m., Jinmei Liao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62163/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2017, 3:33 p.m.) > > > Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick > Rhomberg. > > > Repository: geode > > > Description > ------- > > * Use HttpOperationInvoker to replace RestHttpOperationInvoker and > SimpleHttpOperationInvoker > * Use one single ShellCommandController to replace all command controllers > * do not allow execution of commands that require client side file data > gathering to be executed only on the locator/server > * deprecate CommandService and CommandStatement > * simplify CommandRequest, delete geode's ClientHttpRequest > * fix tests > > > Diffs > ----- > > > geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/GfshStartLocatorLogTest.java > eadbea8a2ddde34aed7e1a39e4826be4c70fd65f > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshExecution.java > 23f2a73acf2cf92a8b1c0c2ea2afd10392265628 > geode-core/build.gradle 8145a634ae706d90026ee0154bdb2eab39e956d0 > geode-core/src/main/java/org/apache/geode/internal/lang/Initializer.java > 20373710390f7496831507684504804c81cff4ee > > geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java > df82dffb8e6655785e5347d99032a64cc4d3b40e > geode-core/src/main/java/org/apache/geode/management/MemberMXBean.java > ca7c2a24f1e78ab2cb3047f06f553b117fc8ba8e > geode-core/src/main/java/org/apache/geode/management/cli/CliMetaData.java > 226086f7c601292bef307313775ae26b97ce65a5 > > geode-core/src/main/java/org/apache/geode/management/cli/CommandService.java > 20f1c75e06dd7e9fa533182274c45db230170da9 > > geode-core/src/main/java/org/apache/geode/management/cli/CommandStatement.java > a01f08c2f09b9c762bbd4ef561ce0ba26d22dd73 > > geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBean.java > 271dce150fb3a93803806700cee7053f9422a8f2 > > geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java > 5105c3d4bd8b2cfd3a2daf0e0f8208115591c8f1 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java > 3c8f6cf235d0f1b34d948d23e39fddfbe306be2c > > geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandRequest.java > 00a05872a512f294f914dbc8ac1c12b799a9145d > > geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponse.java > 81c49583b759ea815f6f20fb1c6da7edb7f99b2f > > geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponseBuilder.java > 790e54be47673f67060d41b323f4b6d22800a852 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParseResult.java > b228b281fb471f699c642045d9603ea9d8f9bfcc > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java > a75eeb0ce591a9e3e1c4f56626a1cae8fe722806 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommand.java > 4f465399abcbcf1d508e05c7fbd73bdd3c68cf1d > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportConfigCommand.java > 672ec881d04d7aa47e01d58268d3df90a285d95a > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportImportClusterConfigurationCommands.java > 83eddeebb472758944863cde098746c7ff8da5a4 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > 70e1e60ff6ed137900eae5d19d67497a5fa718e2 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshCommand.java > c7f53b1552b45f340b244828cb76d09c8aaa83da > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryCommand.java > 3c039b3a70cf7b38b4f4af77079f0b5d6a39caf5 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandExecutor.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java > 2464b0065d61b1aafc3b933f5f1a04e90e95c689 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandStatementImpl.java > ac510d1755c7dba1c2c7a772887a5c1b64cdcf57 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/MemberCommandService.java > 25ff549be2bf706c3e3a312fa1b6ce6b423996e7 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessor.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/RemoteExecutionStrategy.java > 75dce477ba6ce0352ff2575daa7b16f66f1acf18 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/AbstractResultData.java > 0bb96cf7058ad73775ce85008492024987bae346 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java > bbb59d0755ffd2cf405f78c89b420a5279844e29 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/DownloadFileResult.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ErrorResultData.java > 7ae0d8083600bba89eb625b95408afc9b9896059 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/InfoResultData.java > f399a54159d3e1d8b8a68ec59a8ee68c5ca4bbe7 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ResultBuilder.java > 251879396bf5510be9e8208040784c8a8b25fbf6 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java > 2d55f42865afe71e242cd18f3a0ef4feea095693 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java > 735143f9aedf913ea47b128152ac5f2942362de0 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java > d2407b3608cc5e55060588ea5dec6851934ae399 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/ScriptExecutionDetails.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/AbstractCommandsController.java > 0a18ec522884f5f737045b0da53a0e49e1cd6aa9 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ClientCommandsController.java > e8df505f35bf751fa5a57acb3cd1d75ea2e0e35c > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ClusterCommandsController.java > fae10debac618594d191f08e9b45b85e71769a32 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ConfigCommandsController.java > d223a9ff2aa40444ba3aebb3f21d40a72e144546 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java > 3c58f502dad9ac77eea3dc4facef02d7c444add4 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DeployCommandsController.java > 9d3e086af073ee0a81cc639d2295f85af2f343c0 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DiskStoreCommandsController.java > 1f646d6869788ae713cef5de6330d48d74c912a1 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DurableClientCommandsController.java > 0e62e71f194bb498e4f398df5a875bc6616cb7bb > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java > 87afc24a71394d0b293cd2cc05895b5b9e646b34 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/FunctionCommandsController.java > e8885da195dd4486daace457ba07bd185ce0fc25 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/IndexCommandsController.java > 09d7f9a9873c8bc0a928625d8d4a80151d2dc08b > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/LauncherLifecycleCommandsController.java > 2e1084211e80da580f5ba25874f2476c62b8cb0d > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MemberCommandsController.java > c9e7900e45a4890c206f6367448ac4cbd96e487f > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MiscellaneousCommandsController.java > 7bce3d05c7c98b479fbb7a4994ad34be93682132 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/PdxCommandsController.java > dd84444f18aa4227f32cf6bbf3acad45f0a4c0f7 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/QueueCommandsController.java > 9fea79ab866cc9666b049cd9e81980b7d785323c > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/RegionCommandsController.java > 9180414a46aad707ba9ee3de8aa0eaefc7faad48 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ShellCommandsController.java > 63413b0ba5308d4cd06db223e813cdf8f0841d32 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/WanCommandsController.java > 36b4d2e1a8edd6ae60203100040cecbe9df9fd0c > > geode-core/src/main/java/org/apache/geode/management/internal/web/domain/Link.java > 9058eaa428c799ed77afc2f27390749bd86f184e > > geode-core/src/main/java/org/apache/geode/management/internal/web/domain/LinkIndex.java > 2a99e827b845a10a962199eeb8253f2f6c966cb5 > > geode-core/src/main/java/org/apache/geode/management/internal/web/http/ClientHttpRequest.java > 7c60c9aa8a2285da1e5af906af4c2fe5c01f55ab > > geode-core/src/main/java/org/apache/geode/management/internal/web/http/HttpHeader.java > 74836bc6474ded005983b5a992cdfb815699669f > > geode-core/src/main/java/org/apache/geode/management/internal/web/http/HttpMethod.java > 2ba364be82383f8c22f710feca0f8bf44252255e > > geode-core/src/main/java/org/apache/geode/management/internal/web/http/support/SimpleHttpRequester.java > 89adc202616ac5d906abfb361e3c1588acb0ac5d > > geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java > 63eb977278da481b2858e03769175500a147a573 > > geode-core/src/main/java/org/apache/geode/management/internal/web/shell/HttpOperationInvoker.java > a3ce5548ef7a50d6af7bd143d1c2b49301232c15 > > geode-core/src/main/java/org/apache/geode/management/internal/web/shell/RestHttpOperationInvoker.java > 13fd42c41f47b4693e03692052371c14e8512a04 > > geode-core/src/main/java/org/apache/geode/management/internal/web/shell/SimpleHttpOperationInvoker.java > d11d8245b4f164ba4e148d87ea4661eb7f15176c > > geode-core/src/main/java/org/apache/geode/management/internal/web/util/ConvertUtils.java > 0b6fbe32001834f6cec388e040e13f90666dd60f > > geode-core/src/test/java/org/apache/geode/internal/lang/InitializerJUnitTest.java > 66e8ff1f74635127b95462f390b04328ac20dc57 > > geode-core/src/test/java/org/apache/geode/internal/util/ArgumentRedactorJUnitTest.java > b40d4852a37b9f04ee60b011d61521317ec9c450 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandRequestTest.java > 0d6e7cc7acf3695a6f99715f4d4615c743243801 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java > 0357105be62924d94e1ee36349a5d9ef62826245 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/HeadlessGfsh.java > f636c89ca7be4ca88ff0bae9ff6d0f377a643600 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ConfigCommandsDUnitTest.java > a4f523c388f50b0a3d076952e4b906232730d960 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployWithGroupsDUnitTest.java > a89121f36fed256d2a86ddc70938c8aa24f62358 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GfshCommandJUnitTest.java > f6c7caec6f314a573ef0c6e8b2dc691ea55a9642 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShellCommandsDUnitTest.java > 255013e1a22dcd4ff8dc37726208f788656d47b7 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowMetricsDUnitTest.java > b804099c7effd091e6980ba8dbc41d17c715704e > > geode-core/src/test/java/org/apache/geode/management/internal/cli/remote/CommandExecutorTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessorTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/result/CommandResultTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyJUnitTest.java > ece0c7eea85833369cfe8f9edf4756ca7724c7e2 > > geode-core/src/test/java/org/apache/geode/management/internal/security/DeployCommandsSecurityTest.java > 9dd4d991e647749d335e304ab7a2a807142ce68b > > geode-core/src/test/java/org/apache/geode/management/internal/security/MultiUserDUnitTest.java > e3fe173e7d00686df5bf3b9e3212f3dca4c161c7 > > geode-core/src/test/java/org/apache/geode/management/internal/web/controllers/ExportLogControllerTest.java > bee7db219ebd07460d7e9ee94568e9294e911eac > > geode-core/src/test/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerProcessCommandTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/web/controllers/WanCommandsControllerJUnitTest.java > 59d6ff43b2ed2cc7639ba1d97345eda9b3ece583 > > geode-core/src/test/java/org/apache/geode/management/internal/web/domain/LinkTest.java > ff98b7ef7603552fb36fb5ab96c658eb8883d627 > > geode-core/src/test/java/org/apache/geode/management/internal/web/http/ClientHttpRequestTest.java > 4a5a0d40fc5cfb58b54854ff2b4b4afebe4cf48c > > geode-core/src/test/java/org/apache/geode/management/internal/web/shell/HttpOperationInvokerTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java > a9ce889006800523505dace6e0b4696c9911d205 > geode-junit/build.gradle ccfbb24648991c50283a7664e6ecc549265a8457 > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/CommandOverHttpTest.java > PRE-CREATION > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ConnectCommandWithHttpAndSSLDUnitTest.java > d8ef3a8ae02d1091657c97fd20a44dd46d7a597d > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ConnectCommandWithSecurityTest.java > 838924a3f7a011f3e736d329faf6052202a7ae54 > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/QueryNamesOverHttpDUnitTest.java > dbd29b53f42d61d00f895340d3568455c6ccb890 > > geode-web/src/test/java/org/apache/geode/management/internal/security/LogNoPasswordTest.java > 20359a9cf892b5cec5bdce20a693fb8da90d744c > > geode-web/src/test/java/org/apache/geode/management/internal/web/AbstractWebTestCase.java > 73be12afbcf608d855f6f9d3dcc71509d513edcd > > geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerJUnitTest.java > 2f9214ce46e4095fb2d061e6a05411e8cf0b8db9 > > geode-web/src/test/java/org/apache/geode/management/internal/web/domain/LinkIndexJUnitTest.java > 7308b313ff9c7c089cff5c496dc9be6a672ef457 > > geode-web/src/test/java/org/apache/geode/management/internal/web/domain/LinkJUnitTest.java > 2f3f2dc29d13721aa0a8a845a996eca5f0c502da > > geode-web/src/test/java/org/apache/geode/management/internal/web/http/ClientHttpRequestJUnitTest.java > a95a58e7a496cadf211f925485166483ba06b518 > > geode-web/src/test/java/org/apache/geode/management/internal/web/shell/HttpOperationInvokerMBeanOperationTest.java > PRE-CREATION > > geode-web/src/test/java/org/apache/geode/management/internal/web/shell/HttpOperationInvokerSecurityTest.java > PRE-CREATION > > geode-web/src/test/java/org/apache/geode/management/internal/web/shell/RestHttpOperationInvokerJUnitTest.java > 2bebd2ef2e5edc70b3e90458968ebc6e42a4b597 > > geode-web/src/test/java/org/apache/geode/management/internal/web/shell/SimpleHttpOperationInvokerJUnitTest.java > 4571f2ca2f459d40b5a0d42055cf8b2e9b747220 > > geode-web/src/test/java/org/apache/geode/management/internal/web/util/UriUtilsJUnitTest.java > 9d4dd460a75adb79bea855a1408b8f603241ac63 > > > Diff: https://reviews.apache.org/r/62163/diff/1/ > > > Testing > ------- > > I will put this up for review first. Currently pending last precheckin. > > > Thanks, > > Jinmei Liao > >
