> On March 2, 2017, 7:24 p.m., Kirk Lund wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/CliUtil.java > > Lines 308 (patched) > > <https://reviews.apache.org/r/57243/diff/1/?file=1653690#file1653690line308> > > > > You should create CliUtilTest or CliUtilIntegrationTest and write some > > tests for these new methods.
there are dunit tests around it. Will add more tests. > On March 2, 2017, 7:24 p.m., Kirk Lund wrote: > > 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/diff/1/?file=1653692#file1653692line821> > > > > 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. I extracted the command to its own class, but left the implementation intact for now. We have test and integration tests that tests around LogExporter. Will file a JIRA ticket to make the exportlogcommand more testable. > On March 2, 2017, 7:24 p.m., Kirk Lund wrote: > > 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/diff/1/?file=1653692#file1653692line878> > > > > At a minimum, I'd hoist this "Region region =" out of the for-loop and > > invoke it just once. the region is destroyed by the receiver of the function (because it's a replicated region, once the region is destroyed on the server, it will be destroyed on the locator). We are doing this because if we leave the regions around, then then the put messages will be broadcasted to all the members. We had to choose between the network overhead or region create/destroy overhead. We chose the latter. > On March 2, 2017, 7:24 p.m., Kirk Lund wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java > > Lines 57 (patched) > > <https://reviews.apache.org/r/57243/diff/1/?file=1653693#file1653693line57> > > > > 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. It does use a LogExporter. > On March 2, 2017, 7:24 p.m., Kirk Lund wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java > > Lines 119 (patched) > > <https://reviews.apache.org/r/57243/diff/1/?file=1653693#file1653693line119> > > > > Why is this public? Is this called from other classes? Yes, the LogExportCommand uses it to craete the region on the locator. > On March 2, 2017, 7:24 p.m., Kirk Lund wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java > > Lines 142 (patched) > > <https://reviews.apache.org/r/57243/diff/1/?file=1653693#file1653693line142> > > > > Does this need to be public? yes, the command uses it. > On March 2, 2017, 7:24 p.m., Kirk Lund wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java > > Lines 159 (patched) > > <https://reviews.apache.org/r/57243/diff/1/?file=1653693#file1653693line159> > > > > Can this be made private? the command will need to create the argument. > On March 2, 2017, 7:24 p.m., Kirk Lund wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java > > Lines 208 (patched) > > <https://reviews.apache.org/r/57243/diff/1/?file=1653693#file1653693line208> > > > > Can this be made private? command uese it. - Jinmei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57243/#review167709 ----------------------------------------------------------- On March 2, 2017, 7:26 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, 7:26 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-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 > 76e986df2c69bfde9d7311138f8727610276876f > > 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/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/GfshCommandsSecurityTest.java > d1750c3a5efd636c0f9f47fabe670265bf46f072 > > geode-core/src/test/java/org/apache/geode/management/internal/security/MultiUserDUnitTest.java > 37e7f453dab01dbcfcc26ebacaf3c27cc680a5c8 > > 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/LocatorServerConfigurationRule.java > 0a6396263aa18629b178d82932be014a40c134a9 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > 39c13d000db80bef37563729bc17ae4bcb566153 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorStarterRule.java > 84c660cfe0ab9b7c6ba0bfc08b034ccaf17697f0 > geode-core/src/test/java/org/apache/geode/test/dunit/rules/Member.java > 5f46da21f380a67a8e7f4855da1dfbb3118057ba > > 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/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/2/ > > > Testing > ------- > > precheckin running (hopefully last one) > > > Thanks, > > Jinmei Liao > >
