----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57243/#review167709 -----------------------------------------------------------
I'm going to publish what I have so far for review. This is for page 1 of the diffs. geode-core/src/main/java/org/apache/geode/management/internal/cli/CliAroundInterceptor.java Lines 40 (patched) <https://reviews.apache.org/r/57243/#comment239615> This is an internal class. Can we just delete methods like this instead of deprecating it? geode-core/src/main/java/org/apache/geode/management/internal/cli/CliUtil.java Lines 308 (patched) <https://reviews.apache.org/r/57243/#comment239624> You should create CliUtilTest or CliUtilIntegrationTest and write some tests for these new methods. geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandRequest.java Lines 43 (patched) <https://reviews.apache.org/r/57243/#comment239626> I recommend making this final and alter the constructor: ```java this.downloadFile = metaData != null && metaData.isFileDownloadOverHttp(); ``` geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java Line 782 (original), 694 (patched) <https://reviews.apache.org/r/57243/#comment239635> We should not modify this method in place (in MiscellaneousCommands) without extracting it and introducing unit tests. Example: 1) extract this command to its own class "ExportLogsCommand" 2) extract the guts of this method to another class (maybe called ExportLogs) ExportLogs becomes a simple class that does not know anything about GFSH or CLI parsing or those annotations. It just knows how to do the actual work based on arguments passed to its constructor. ExportLogsCommand becomes a simple class that uses ExportLogs within the context of GFSH and handles all of the CLI annotations. Then we can write a true mocking UnitTest for ExportLogs and test this class and its functionality in isolation from GFSH. All classes should end up being testable with a true UnitTest and classes should be refactored to follow good OOP practices (single responsibility, etc). This is not an OOP class as currently written. This method should also be broken down into nice small methods that can be tested and tests should be written for them. geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java Line 810 (original), 728 (patched) <https://reviews.apache.org/r/57243/#comment239628> At some point we need to delete CliUtil and CacheFactory.getAnyInstance and start passing around instances instead of using statics and singletons. This forces us into doing all of our testing with integration tests instead of unit tests. It's sad to see so many static calls in this method that are invoking getAnyInstance geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java Line 833 (original), 733 (patched) <https://reviews.apache.org/r/57243/#comment239629> At a minimum, I'd hoist this "Region region =" out of the for-loop and invoke it just once. geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java Line 872 (original), 749 (patched) <https://reviews.apache.org/r/57243/#comment239632> Fix typo "Recieved" and change to use log4j parms: ```java logger.info("Received zip file from member {}: {}", server.getId(), zipFile); ``` geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java Lines 57 (patched) <https://reviews.apache.org/r/57243/#comment239637> This class should be a Function that uses another class called LogExporter. LogExporter should be a class that follows solid OOP principles and has a new UnitTest that uses Mockito to test it in isolation. ExportLogsFunction then becomes an integration class that uses LogExporter within the context of a Function. geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java Lines 119 (patched) <https://reviews.apache.org/r/57243/#comment239638> Why is this public? Is this called from other classes? geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java Lines 121 (patched) <https://reviews.apache.org/r/57243/#comment239640> Let's get rid of these singleton calls. Make the methods non-static (and preferrably private). Call GemFireCacheImpl.getInstance() once and store it in a member variable. Then have all of the other methods use that var instead of hitting GemFireCacheImpl.getInstance() repeatedly. We want to remove all statics and singletons so that we can start writing good unit tests. geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java Lines 142 (patched) <https://reviews.apache.org/r/57243/#comment239639> Does this need to be public? geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java Lines 159 (patched) <https://reviews.apache.org/r/57243/#comment239642> Can this be made private? geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java Lines 208 (patched) <https://reviews.apache.org/r/57243/#comment239641> Can this be made private? - Kirk Lund On March 2, 2017, 4:32 p.m., Jinmei Liao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57243/ > ----------------------------------------------------------- > > (Updated March 2, 2017, 4:32 p.m.) > > > Review request for geode, Jared Stewart, Kevin Duling, Ken Howe, and Kirk > Lund. > > > Repository: geode > > > Description > ------- > > GEODE-2414: Determine a mechanism to stream a zip file from server to locator > GEODE-2415: Write a function to return a zip file for a single server > GEODE-2418: enable gfsh to download file from http connection > GEODE-2267: add validation to the arguments and include export stats in the > command > GEODE-2267: use the config to determine where the logs and stats are > GEODE-2267: Enhance server/locator startup rules to include workingDir > > What's not done in this batch: > 1. If you are exporting without http connection, exported zip will be left on > the loator's working dir, currently we don't have a cleanup mechanism to > delete them. > 2. long running task may timeout the http connection. No test around this > scenario. > 3. No warning to the user if they are exporting a large quantity of logs. > 4. we are exporting all the .log and .gfs files and only those files. Other > file extensions are ignored. > 5. merge-log option is ignored > > > Diffs > ----- > > geode-assembly/build.gradle 1c95927f19888b15e771b3cbbc92f120787868c4 > > geode-assembly/src/test/java/org/apache/geode/management/internal/AgentUtilJUnitTest.java > 3a29e8a435f5d5362810f5e05ea92a1260e3fbfc > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityIntegrationTest.java > f0b6bb327f0424f2e4afa2e057d8e38bd0ef1b39 > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityPostProcessorTest.java > 160f6349d94646b12e3114da14e5bff569eed5a9 > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestServersJUnitTest.java > 10ce515df8c3630cbf01fc314be454d3e2adfe2c > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/SwaggerVerificationTest.java > c7d0e7341fcf13bf34a597405d2b42b608004215 > > geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseDataExportTest.java > b5472909eec8d5ca124e7bfd5c6cb71864d9bbee > > geode-core/src/main/java/org/apache/geode/distributed/DistributedSystem.java > 20c948f8f16240aef5c8eae313164693125a99ca > > geode-core/src/main/java/org/apache/geode/internal/logging/GemFireLevel.java > cffd162687573c9f7861bc2ada1076293c2e0a57 > > geode-core/src/main/java/org/apache/geode/internal/logging/InternalLogWriter.java > 661fce99f01a890b727daf7dc591316d1d8982fb > geode-core/src/main/java/org/apache/geode/internal/logging/LogService.java > 0ad7d848a05a0104ce01652b1fb628f2cff1b2d1 > > geode-core/src/main/java/org/apache/geode/internal/statistics/StatArchiveHandler.java > 03b0f898ad30b6b088108fa0187b0a5a750ed6dc > geode-core/src/main/java/org/apache/geode/management/cli/CliMetaData.java > a20fba5a1b9fa185283bd7c60c4ade77345ed4ac > > geode-core/src/main/java/org/apache/geode/management/internal/cli/AbstractCliAroundInterceptor.java > de24727819856d180a69ed0021c21aa016e71712 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/CliAroundInterceptor.java > 11d74c13311923357e82318b32bf0342e156e0c6 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/CliUtil.java > 8cd098d9719774c49efcf28d1d5ecdcd345687fc > > geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandRequest.java > b0242c9e8ee656001cf76155f4e727ece07666a2 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java > bbf0542b80469e11d21a2ed56f52ef9e647e91ea > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/LogFileFunction.java > 41ffeb40b4dd09ed2bf7eb4ef2cd516001927025 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java > c536e8e99ef32043d9caf267dd563ee2d05b6e50 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java > b53c53f91ed63a01bfaa1232d9e194481ae45664 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ExportLogsCacheWriter.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ExportLogsRepository.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogExporter.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogFilter.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogLevelExtractor.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/MergeLogs.java > 8d2ef4599337cb091da6f26c6034b1a71311148d > > geode-core/src/main/java/org/apache/geode/management/internal/configuration/utils/ZipUtils.java > 81161d6f3e2539ca3d09765e99b891d1522da302 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MiscellaneousCommandsController.java > ac912c82c873c443dd268f07e8874b5bd18fdd0b > > geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java > fa052489772e3e03eb865d17dbbcb7e227813c42 > > geode-core/src/main/java/org/apache/geode/management/internal/web/shell/RestHttpOperationInvoker.java > 82b2b1fc0d2c8952835b0101c413161d39f361dc > > geode-core/src/main/java/org/apache/geode/management/internal/web/shell/SimpleHttpOperationInvoker.java > dcda27bef61bad1d7caf1fe1a99063f99d768819 > > geode-core/src/test/java/org/apache/geode/internal/statistics/SimpleStatSamplerIntegrationTest.java > 167fa3d07802136ea68ec2f4f54e7c4716ea938a > > geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java > 83a367eb88aec85984691e651e5de0f8b479c7cb > > geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandRequestTest.java > 543623766b3422d63a1c3ceca38229144c94076b > > geode-core/src/test/java/org/apache/geode/management/internal/cli/HeadlessGfsh.java > 72be0c7fa6839ff7f818cffa4ee5b436bebd2ce8 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java > 21426d67904758aad4b1cf8b8c8966dd0fd45365 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandsDUnitTest.java > 05f2c0cb2115ee574666ed3da943c26e8c38c9f2 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsInterceptorJUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOnServerManagerDUnit.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportStatsDUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommandsExportLogsPart1DUnitTest.java > cb9a8c6f74ba2a7b9799ad049a4ebe2861b9087b > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommandsExportLogsPart2DUnitTest.java > 2b2e524c5369950073099c3509c8f30e768607f6 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommandsExportLogsPart3DUnitTest.java > efef2c4abf70deeaef81285f13997542490234e3 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommandsExportLogsPart4DUnitTest.java > 9d64bd9ed3fec910c2b5dd4e8be50d3368c63893 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunctionIntegrationTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/ExportLogsCacheWriterUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogExporterIntegrationTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogExporterTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogFilterTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogLevelExtractorTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/MergeLogsDUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/EventTestCacheWriter.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ZipUtilsJUnitTest.java > 1791574bc688774900ba2e609f3c80600cb5cac9 > > geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerMBeanShiroJUnitTest.java > 3183aafd9907ecb15714c2038f2b8c3af31e6e06 > > geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerStartupRule.java > ea72cfab26f828af9a09f08755a9a7f9ed121654 > > geode-core/src/test/java/org/apache/geode/management/internal/security/DeployCommandsSecurityTest.java > e89bd62b59b8218c30c8f724b4febf2b3ae7721b > > geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsPostProcessorTest.java > 0980c18de1b504f545355a0f7d650f46c92cc670 > > geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java > 2d32905d45496fe69e5dc189cd94b82ea163c3b5 > > geode-core/src/test/java/org/apache/geode/management/internal/security/JavaRmiServerNameTest.java > 2ae61402d330ab6426c0bc0e0193771ce5dc9c70 > > geode-core/src/test/java/org/apache/geode/management/internal/security/MultiUserDUnitTest.java > 37e7f453dab01dbcfcc26ebacaf3c27cc680a5c8 > > geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorDUnitTest.java > 52afab40990ab3a18fed04fcb001ec9dbe7d045e > > geode-core/src/test/java/org/apache/geode/security/PeerSecurityWithEmbeddedLocatorDUnitTest.java > e87a285d088c923326aa82ee406f543ffc5e0593 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java > 4729be3f4d9b51168422784a57ac1ec76e018e83 > geode-core/src/test/java/org/apache/geode/test/dunit/rules/Locator.java > fe26e8364798689c1ece487554d99085f3f26f23 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerConfigurationRule.java > 0a6396263aa18629b178d82932be014a40c134a9 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > 68447201875ef31a73cfb888d346292e3f084ae8 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorStarterRule.java > c3e493e1c176974ed757c71d5c195ff5964dbb57 > geode-core/src/test/java/org/apache/geode/test/dunit/rules/Member.java > 7cc1eea2e26b176b570111bccdb8914e19528ecf > geode-core/src/test/java/org/apache/geode/test/dunit/rules/Server.java > 4aa2c69389dce4650cf476f4e8decebddfc36736 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java > b5ddee666c52fe39deb015c6e2aa57e5b21e4f20 > > geode-core/src/test/resources/org/apache/geode/codeAnalysis/sanctionedSerializables.txt > 8c0fb4569d0fbfa99622a18bac41281b1cc561df > > geode-core/src/test/resources/org/apache/geode/management/internal/cli/commands/golden-help-offline.properties > 694d4ffd2c01b0e99010fa425fcbeac08029843b > geode-web/build.gradle ffab391d97792ce82fa21fecbc00afb45fe820d6 > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/CommandOverHttpDUnitTest.java > d4869c2110f69275c566eb52e5d09a38ebe30589 > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpDUnitTest.java > PRE-CREATION > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/QueryNamesOverHttpDUnitTest.java > 1a19e5e5ca90c248655eb40e68aca3c7ed858770 > > geode-web/src/test/java/org/apache/geode/management/internal/web/shell/RestHttpOperationInvokerJUnitTest.java > 808c78f0f4a39501563a389e8b156452c48ffe81 > > geode-web/src/test/java/org/apache/geode/management/internal/web/shell/SimpleHttpOperationInvokerJUnitTest.java > 8afbcf683824545dff050d88ef320a260855c056 > > > Diff: https://reviews.apache.org/r/57243/diff/1/ > > > Testing > ------- > > precheckin running (hopefully last one) > > > Thanks, > > Jinmei Liao > >
