geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ken Howe <kh...@pivotal.io>
Subject Re: Review Request 59811: GEODE-2420: add file-size-limit param to the ExportLogsController
Date Wed, 07 Jun 2017 22:30:56 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message