> On June 7, 2017, 9:40 p.m., Jared Stewart wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > > Line 122 (original), 123 (patched) > > <https://reviews.apache.org/r/59811/diff/2/?file=1743972#file1743972line123> > > > > I really like the "fail fast" idea! Can I suggest a minor reordering > > of a few lines that might be more readable? (The basic idea is just to > > move the lines into a narrower scope when possible). > > > > ``` > > List<Object> results = (List<Object>) estimateLogSize(args, > > server).getResult(); > > if (!results.isEmpty()) { > > if (results.get(0) instanceof Long) { > > long estimatedSize = (Long) results.get(0); > > logger.info("Received estimated export size from member > > {}: {}", server.getId(), > > estimatedSize); > > totalEstimatedExportSize += estimatedSize; > > } else if (results.get(0) instanceof ManagementException) { > > ManagementException exception = (ManagementException) > > results.get(0); > > return > > ResultBuilder.createUserErrorResult(exception.getMessage()); > > } > > } > > ```
Good suggestion, I do think it helps readability. - Ken ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59811/#review177230 ----------------------------------------------------------- On June 7, 2017, 10:23 p.m., Ken Howe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59811/ > ----------------------------------------------------------- > > (Updated June 7, 2017, 10:23 p.m.) > > > Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, > and Patrick Rhomberg. > > > Repository: geode > > > Description > ------- > > GEODE-2420: add file-size-limit param to the ExportLogsController > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > 0ff780cbf66937d8ececfb3a2d0789ee485b9b62 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java > 57355c0efae4c6da9470267f95e27e59aa4d8b2c > > geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java > 9f68d3a5eaadbe8f1bd95ec8df85ed1f65aa67ce > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java > a369c6e1ffb330715fbde2cd69d023ed36f133ad > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java > 16549e70bbebf4390bb73a481274e92ca6cad035 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java > 8609b3aaf0a0eb1ba903bd39c64103f9510a6a78 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java > 09ee08dd6af29b9a418ef7499defc4980da787ed > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsStatsDUnitTest.java > 44a036298e0991c880fc552596d296e104b97ca1 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java > 4e1dac013d239437829bc52dc70689c4ba15dc58 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionTest.java > cc5e7d5256741ad0a48ff87c7f989a18b90f7f03 > > > Diff: https://reviews.apache.org/r/59811/diff/2/ > > > Testing > ------- > > 6/7/17: re-started precheckin > precheckin is green with the exception of unrelated DUnit tests > > Precheckin started > > > Thanks, > > Ken Howe > >
