You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Patrick Rhomberg <pr...@pivotal.io> on 2017/03/28 22:31:54 UTC
Review Request 58010: GEODE-2716: export logs default behavior
changed from filtering at log level INFO to ALL.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58010/
-----------------------------------------------------------
Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
Repository: geode
Description
-------
Added DUnit test that fails under previous behavior.
Diffs
-----
geode-core/src/main/java/org/apache/geode/internal/logging/LogService.java 1f8a564a8111b036f5e5cce6393931c714985c9c
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java cbdf1c4bc28554a8fbec3740c566ee07c69b4ac9
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 95edd426da8b8f39bb1486661d8c307d43f170d6
Diff: https://reviews.apache.org/r/58010/diff/1/
Testing
-------
Added DUnit passes under new behavior.
Thanks,
Patrick Rhomberg
Re: Review Request 58010: GEODE-2716: export logs default behavior
changed from filtering at log level INFO to ALL.
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58010/#review170882
-----------------------------------------------------------
Ship it!
Ship It!
- Jinmei Liao
On March 30, 2017, 7:19 p.m., Patrick Rhomberg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58010/
> -----------------------------------------------------------
>
> (Updated March 30, 2017, 7:19 p.m.)
>
>
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added relevant unit and dUnit tests. Moved default value variable to ExportLogsCommand and deleted the now-unused LogService.DEFAULT_LOG_LEVEL
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/logging/LogService.java 1f8a564
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java 3f147c1
> geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java cbdf1c4
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 95edd42
> geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunctionTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/58010/diff/2/
>
>
> Testing
> -------
>
> precheckin running.
>
>
> Thanks,
>
> Patrick Rhomberg
>
>
Re: Review Request 58010: GEODE-2716: export logs default behavior
changed from filtering at log level INFO to ALL.
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58010/#review170789
-----------------------------------------------------------
Ship it!
Ship It!
- Kirk Lund
On March 30, 2017, 7:19 p.m., Patrick Rhomberg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58010/
> -----------------------------------------------------------
>
> (Updated March 30, 2017, 7:19 p.m.)
>
>
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added relevant unit and dUnit tests. Moved default value variable to ExportLogsCommand and deleted the now-unused LogService.DEFAULT_LOG_LEVEL
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/logging/LogService.java 1f8a564
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java 3f147c1
> geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java cbdf1c4
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 95edd42
> geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunctionTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/58010/diff/2/
>
>
> Testing
> -------
>
> precheckin running.
>
>
> Thanks,
>
> Patrick Rhomberg
>
>
Re: Review Request 58010: GEODE-2716: export logs default behavior
changed from filtering at log level INFO to ALL.
Posted by Patrick Rhomberg <pr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58010/
-----------------------------------------------------------
(Updated March 30, 2017, 7:19 p.m.)
Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
Changes
-------
Added simpler class unit tests, updated variable name and location.
Repository: geode
Description (updated)
-------
Added relevant unit and dUnit tests. Moved default value variable to ExportLogsCommand and deleted the now-unused LogService.DEFAULT_LOG_LEVEL
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/internal/logging/LogService.java 1f8a564
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java 3f147c1
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java cbdf1c4
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 95edd42
geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunctionTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/58010/diff/2/
Changes: https://reviews.apache.org/r/58010/diff/1-2/
Testing (updated)
-------
precheckin running.
Thanks,
Patrick Rhomberg
Re: Review Request 58010: GEODE-2716: export logs default behavior
changed from filtering at log level INFO to ALL.
Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58010/#review170464
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java
Line 170 (original), 170 (patched)
<https://reviews.apache.org/r/58010/#comment243283>
I would prefer to set the default value set here directly rather than to reference a value inside LogService.
- Jared Stewart
On March 28, 2017, 10:34 p.m., Patrick Rhomberg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58010/
> -----------------------------------------------------------
>
> (Updated March 28, 2017, 10:34 p.m.)
>
>
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added DUnit test that fails under previous behavior.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/logging/LogService.java 1f8a564a8111b036f5e5cce6393931c714985c9c
> geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java cbdf1c4bc28554a8fbec3740c566ee07c69b4ac9
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 95edd426da8b8f39bb1486661d8c307d43f170d6
>
>
> Diff: https://reviews.apache.org/r/58010/diff/1/
>
>
> Testing
> -------
>
> Added DUnit passes under new behavior.
>
> ===
>
> While `LogService.DEFAULT_LOG_LEVEL` is only used by the export logs command, I don't know if that is the best place to make this update. Should that stay `INFO` and a new variable `DEFAULT_EXPORT_LOG_LEVEL` be established?
>
>
> Thanks,
>
> Patrick Rhomberg
>
>
Re: Review Request 58010: GEODE-2716: export logs default behavior
changed from filtering at log level INFO to ALL.
Posted by Ken Howe <kh...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58010/#review170470
-----------------------------------------------------------
I would add a new constant for DEFAULT_EXPORT_LOG_LEVEL, and move it to ExportLogsCommand.
This makes it completely clear that the level we're filtering on for exporting the logs is completely different than any level for what is getting logged.
I also think that LogService.DEFAULT_LOG_LEVEL should be removed; it's internal and not used anywhere (after the change noted above.)
- Ken Howe
On March 28, 2017, 10:34 p.m., Patrick Rhomberg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58010/
> -----------------------------------------------------------
>
> (Updated March 28, 2017, 10:34 p.m.)
>
>
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added DUnit test that fails under previous behavior.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/logging/LogService.java 1f8a564a8111b036f5e5cce6393931c714985c9c
> geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java cbdf1c4bc28554a8fbec3740c566ee07c69b4ac9
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 95edd426da8b8f39bb1486661d8c307d43f170d6
>
>
> Diff: https://reviews.apache.org/r/58010/diff/1/
>
>
> Testing
> -------
>
> Added DUnit passes under new behavior.
>
> ===
>
> While `LogService.DEFAULT_LOG_LEVEL` is only used by the export logs command, I don't know if that is the best place to make this update. Should that stay `INFO` and a new variable `DEFAULT_EXPORT_LOG_LEVEL` be established?
>
>
> Thanks,
>
> Patrick Rhomberg
>
>
Re: Review Request 58010: GEODE-2716: export logs default behavior
changed from filtering at log level INFO to ALL.
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58010/#review170493
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/logging/LogService.java
Line 49 (original), 49 (patched)
<https://reviews.apache.org/r/58010/#comment243328>
Move this constant into ExportLogCommand class
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java
Line 156 (original), 156 (patched)
<https://reviews.apache.org/r/58010/#comment243330>
I recommend creating a new ExportLogsFunctionArgsTest which is a UnitTest for this ExportLogsFunction.Args class.
At a minimum you create a test which confirms the default log level:
```java
@Test
public void defaultLogLevelShouldBeAll() throws Exception {
...
assertThat(logLeve).isEqualTo(ALL);
}
```
Better yet, use this as practice for TDD. Remove your log level change and write this test so it fails BEFORE changing the default log level. Then change the default so the test passes.
Define any additional tests that you think are valuable (use "", use null, use all defaults). Don't go crazy with tons of tests just some basic tests. Any class worth having is worth having a basic unit test for it.
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java
Line 156 (original), 156 (patched)
<https://reviews.apache.org/r/58010/#comment243331>
- Kirk Lund
On March 28, 2017, 10:34 p.m., Patrick Rhomberg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58010/
> -----------------------------------------------------------
>
> (Updated March 28, 2017, 10:34 p.m.)
>
>
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added DUnit test that fails under previous behavior.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/logging/LogService.java 1f8a564a8111b036f5e5cce6393931c714985c9c
> geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java cbdf1c4bc28554a8fbec3740c566ee07c69b4ac9
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 95edd426da8b8f39bb1486661d8c307d43f170d6
>
>
> Diff: https://reviews.apache.org/r/58010/diff/1/
>
>
> Testing
> -------
>
> Added DUnit passes under new behavior.
>
> ===
>
> While `LogService.DEFAULT_LOG_LEVEL` is only used by the export logs command, I don't know if that is the best place to make this update. Should that stay `INFO` and a new variable `DEFAULT_EXPORT_LOG_LEVEL` be established?
>
>
> Thanks,
>
> Patrick Rhomberg
>
>
Re: Review Request 58010: GEODE-2716: export logs default behavior
changed from filtering at log level INFO to ALL.
Posted by Patrick Rhomberg <pr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58010/
-----------------------------------------------------------
(Updated March 28, 2017, 10:34 p.m.)
Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
Repository: geode
Description
-------
Added DUnit test that fails under previous behavior.
Diffs
-----
geode-core/src/main/java/org/apache/geode/internal/logging/LogService.java 1f8a564a8111b036f5e5cce6393931c714985c9c
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java cbdf1c4bc28554a8fbec3740c566ee07c69b4ac9
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 95edd426da8b8f39bb1486661d8c307d43f170d6
Diff: https://reviews.apache.org/r/58010/diff/1/
Testing (updated)
-------
Added DUnit passes under new behavior.
===
While `LogService.DEFAULT_LOG_LEVEL` is only used by the export logs command, I don't know if that is the best place to make this update. Should that stay `INFO` and a new variable `DEFAULT_EXPORT_LOG_LEVEL` be established?
Thanks,
Patrick Rhomberg