You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Bruce Schuchardt <bs...@pivotal.io> on 2016/05/20 17:00:06 UTC
Review Request 47650: GEODE-1415 Disable logging of banners in
distributedTests
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47650/
-----------------------------------------------------------
Review request for geode, Darrel Schneider, Hitesh Khamesra, Jianxia Chen, and Udo Kohlmeyer.
Bugs: GEODE-1415
https://issues.apache.org/jira/browse/GEODE-1415
Repository: geode
Description
-------
Banners are normally logged during creation of an InternalDistributedSystem. In DUnit tests the output is filled with these banners and they all have the same information, so they are just clutter.
This change-set disables banners in non-locator VMs launched by DUnitLauncher/ProcessManager. Precheckin testing shows that the amount of output for geode-core/distributedTests alone used to be over 1gb and is now less than 500mb.
Probably the only change here that requires much scrutiny is in LogWriterFactory. Darrel, I couldn't figure out what the isLoner check is there for.
There are a lot of diffs in Banner.java that you can ignore - I found that it was using tabs for indentation and changed these to spaces. There are no other changes to that class.
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/admin/internal/AdminDistributedSystemImpl.java 21a9ae3c14b697a94787f3236f441046ca639d55
geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalLocator.java cf8d91ee9dc12224b8074d6c4dc8c738c00a3b0f
geode-core/src/main/java/com/gemstone/gemfire/internal/Banner.java 4b01e54dff6f11b06f85165ca843e5a5f7b606ad
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/properties.html 1cd1b72aa8d2e312c44b435bc215885b2a312489
geode-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java d9352c0c90873750402ef66ad07bec2f12935198
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/DUnitLauncher.java 0c600ab57cad5e6443e340f1b6a160ae39b966cb
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/ProcessManager.java 5dbdfd6dee85e2de1d3c993e4ad096c5dcc1ed40
Diff: https://reviews.apache.org/r/47650/diff/
Testing
-------
precheckin had three unrelated failures that reproduce consistently without these changes.
Thanks,
Bruce Schuchardt
Re: Review Request 47650: GEODE-1415 Disable logging of banners in
distributedTests
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47650/#review134196
-----------------------------------------------------------
Ship it!
Ship It!
- Kirk Lund
On May 20, 2016, 6:48 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47650/
> -----------------------------------------------------------
>
> (Updated May 20, 2016, 6:48 p.m.)
>
>
> Review request for geode, Darrel Schneider, Hitesh Khamesra, Jianxia Chen, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1415
> https://issues.apache.org/jira/browse/GEODE-1415
>
>
> Repository: geode
>
>
> Description
> -------
>
> Banners are normally logged during creation of an InternalDistributedSystem. In DUnit tests the output is filled with these banners and they all have the same information, so they are just clutter.
>
> This change-set disables banners in non-locator VMs launched by DUnitLauncher/ProcessManager. Precheckin testing shows that the amount of output for geode-core/distributedTests alone used to be over 1gb and is now less than 500mb.
>
> Probably the only change here that requires much scrutiny is in LogWriterFactory. Darrel, I couldn't figure out what the isLoner check is there for.
>
> There are a lot of diffs in Banner.java that you can ignore - I found that it was using tabs for indentation and changed these to spaces. There are no other changes to that class.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/admin/internal/AdminDistributedSystemImpl.java 21a9ae3c14b697a94787f3236f441046ca639d55
> geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalLocator.java cf8d91ee9dc12224b8074d6c4dc8c738c00a3b0f
> geode-core/src/main/java/com/gemstone/gemfire/internal/Banner.java 4b01e54dff6f11b06f85165ca843e5a5f7b606ad
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/properties.html 1cd1b72aa8d2e312c44b435bc215885b2a312489
> geode-core/src/main/java/com/gemstone/gemfire/internal/logging/LogWriterFactory.java d9352c0c90873750402ef66ad07bec2f12935198
> geode-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java 69df873674632a8908a022d8220dcb2f329de375
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/DUnitLauncher.java 0c600ab57cad5e6443e340f1b6a160ae39b966cb
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/ProcessManager.java 5dbdfd6dee85e2de1d3c993e4ad096c5dcc1ed40
>
> Diff: https://reviews.apache.org/r/47650/diff/
>
>
> Testing
> -------
>
> precheckin had three unrelated failures that reproduce consistently without these changes.
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 47650: GEODE-1415 Disable logging of banners in
distributedTests
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47650/
-----------------------------------------------------------
(Updated May 20, 2016, 6:48 p.m.)
Review request for geode, Darrel Schneider, Hitesh Khamesra, Jianxia Chen, and Udo Kohlmeyer.
Changes
-------
uploaded a new diff since the old one wasn't displaying correctly
Bugs: GEODE-1415
https://issues.apache.org/jira/browse/GEODE-1415
Repository: geode
Description
-------
Banners are normally logged during creation of an InternalDistributedSystem. In DUnit tests the output is filled with these banners and they all have the same information, so they are just clutter.
This change-set disables banners in non-locator VMs launched by DUnitLauncher/ProcessManager. Precheckin testing shows that the amount of output for geode-core/distributedTests alone used to be over 1gb and is now less than 500mb.
Probably the only change here that requires much scrutiny is in LogWriterFactory. Darrel, I couldn't figure out what the isLoner check is there for.
There are a lot of diffs in Banner.java that you can ignore - I found that it was using tabs for indentation and changed these to spaces. There are no other changes to that class.
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/admin/internal/AdminDistributedSystemImpl.java 21a9ae3c14b697a94787f3236f441046ca639d55
geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalLocator.java cf8d91ee9dc12224b8074d6c4dc8c738c00a3b0f
geode-core/src/main/java/com/gemstone/gemfire/internal/Banner.java 4b01e54dff6f11b06f85165ca843e5a5f7b606ad
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/properties.html 1cd1b72aa8d2e312c44b435bc215885b2a312489
geode-core/src/main/java/com/gemstone/gemfire/internal/logging/LogWriterFactory.java d9352c0c90873750402ef66ad07bec2f12935198
geode-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java 69df873674632a8908a022d8220dcb2f329de375
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/DUnitLauncher.java 0c600ab57cad5e6443e340f1b6a160ae39b966cb
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/ProcessManager.java 5dbdfd6dee85e2de1d3c993e4ad096c5dcc1ed40
Diff: https://reviews.apache.org/r/47650/diff/
Testing
-------
precheckin had three unrelated failures that reproduce consistently without these changes.
Thanks,
Bruce Schuchardt
Re: Review Request 47650: GEODE-1415 Disable logging of banners in
distributedTests
Posted by Bruce Schuchardt <bs...@pivotal.io>.
> On May 20, 2016, 6:36 p.m., Darrel Schneider wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalLocator.java, line 123
> > <https://reviews.apache.org/r/47650/diff/1/?file=1389108#file1389108line123>
> >
> > What was the reason for changing the prefix on this system property?
> > I think we want to get rid of this "gemfire." prefix in geode. The old "Locator." prefix would work in both gemfire and geode.
>
> Bruce Schuchardt wrote:
> The "Locator" part of this property is inaccurate because it works in any type of member. As we haven't decided to move from "gemfire." prefixes to "geode." yet I used the former. If we decide to move to "geode." property prefixes we'll need to make a sweep and change them throughout the source code & this one will be changed at that time.
I've reverted the name to Locator.INHIBIT_DM_BANNER to avoid any controversy
- Bruce
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47650/#review134180
-----------------------------------------------------------
On May 20, 2016, 6:48 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47650/
> -----------------------------------------------------------
>
> (Updated May 20, 2016, 6:48 p.m.)
>
>
> Review request for geode, Darrel Schneider, Hitesh Khamesra, Jianxia Chen, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1415
> https://issues.apache.org/jira/browse/GEODE-1415
>
>
> Repository: geode
>
>
> Description
> -------
>
> Banners are normally logged during creation of an InternalDistributedSystem. In DUnit tests the output is filled with these banners and they all have the same information, so they are just clutter.
>
> This change-set disables banners in non-locator VMs launched by DUnitLauncher/ProcessManager. Precheckin testing shows that the amount of output for geode-core/distributedTests alone used to be over 1gb and is now less than 500mb.
>
> Probably the only change here that requires much scrutiny is in LogWriterFactory. Darrel, I couldn't figure out what the isLoner check is there for.
>
> There are a lot of diffs in Banner.java that you can ignore - I found that it was using tabs for indentation and changed these to spaces. There are no other changes to that class.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/admin/internal/AdminDistributedSystemImpl.java 21a9ae3c14b697a94787f3236f441046ca639d55
> geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalLocator.java cf8d91ee9dc12224b8074d6c4dc8c738c00a3b0f
> geode-core/src/main/java/com/gemstone/gemfire/internal/Banner.java 4b01e54dff6f11b06f85165ca843e5a5f7b606ad
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/properties.html 1cd1b72aa8d2e312c44b435bc215885b2a312489
> geode-core/src/main/java/com/gemstone/gemfire/internal/logging/LogWriterFactory.java d9352c0c90873750402ef66ad07bec2f12935198
> geode-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java 69df873674632a8908a022d8220dcb2f329de375
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/DUnitLauncher.java 0c600ab57cad5e6443e340f1b6a160ae39b966cb
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/ProcessManager.java 5dbdfd6dee85e2de1d3c993e4ad096c5dcc1ed40
>
> Diff: https://reviews.apache.org/r/47650/diff/
>
>
> Testing
> -------
>
> precheckin had three unrelated failures that reproduce consistently without these changes.
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 47650: GEODE-1415 Disable logging of banners in
distributedTests
Posted by Bruce Schuchardt <bs...@pivotal.io>.
> On May 20, 2016, 6:36 p.m., Darrel Schneider wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalLocator.java, line 123
> > <https://reviews.apache.org/r/47650/diff/1/?file=1389108#file1389108line123>
> >
> > What was the reason for changing the prefix on this system property?
> > I think we want to get rid of this "gemfire." prefix in geode. The old "Locator." prefix would work in both gemfire and geode.
The "Locator" part of this property is inaccurate because it works in any type of member. As we haven't decided to move from "gemfire." prefixes to "geode." yet I used the former. If we decide to move to "geode." property prefixes we'll need to make a sweep and change them throughout the source code & this one will be changed at that time.
- Bruce
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47650/#review134180
-----------------------------------------------------------
On May 20, 2016, 5 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47650/
> -----------------------------------------------------------
>
> (Updated May 20, 2016, 5 p.m.)
>
>
> Review request for geode, Darrel Schneider, Hitesh Khamesra, Jianxia Chen, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1415
> https://issues.apache.org/jira/browse/GEODE-1415
>
>
> Repository: geode
>
>
> Description
> -------
>
> Banners are normally logged during creation of an InternalDistributedSystem. In DUnit tests the output is filled with these banners and they all have the same information, so they are just clutter.
>
> This change-set disables banners in non-locator VMs launched by DUnitLauncher/ProcessManager. Precheckin testing shows that the amount of output for geode-core/distributedTests alone used to be over 1gb and is now less than 500mb.
>
> Probably the only change here that requires much scrutiny is in LogWriterFactory. Darrel, I couldn't figure out what the isLoner check is there for.
>
> There are a lot of diffs in Banner.java that you can ignore - I found that it was using tabs for indentation and changed these to spaces. There are no other changes to that class.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/admin/internal/AdminDistributedSystemImpl.java 21a9ae3c14b697a94787f3236f441046ca639d55
> geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalLocator.java cf8d91ee9dc12224b8074d6c4dc8c738c00a3b0f
> geode-core/src/main/java/com/gemstone/gemfire/internal/Banner.java 4b01e54dff6f11b06f85165ca843e5a5f7b606ad
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/properties.html 1cd1b72aa8d2e312c44b435bc215885b2a312489
> geode-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java d9352c0c90873750402ef66ad07bec2f12935198
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/DUnitLauncher.java 0c600ab57cad5e6443e340f1b6a160ae39b966cb
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/ProcessManager.java 5dbdfd6dee85e2de1d3c993e4ad096c5dcc1ed40
>
> Diff: https://reviews.apache.org/r/47650/diff/
>
>
> Testing
> -------
>
> precheckin had three unrelated failures that reproduce consistently without these changes.
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 47650: GEODE-1415 Disable logging of banners in
distributedTests
Posted by Kirk Lund <ki...@gmail.com>.
On May 20, 2016, 6:36 p.m., Bruce Schuchardt wrote:
> > I didn't see anything related to isLoner and I didn't see any diff for LogWriterFactory to look at.
> > Going back and looking at the LogWriterFactory I don't understand the !isLoner code in it.
> > The old ticket (35602) had to do with loners never logging a banner so I would have thought this code
> > would need to say something like "if isLoner then logBanner". But instead it is "if !isLoner then logBanner".
> > But I didn't see any problems with the changes you are making.
My confusion with LogWriterFactory is independent of Bruce's change, but maybe now is a good time to address it as well?
I'm not sure why LogWriterFactory needs boolean isLoner (supposedly to fix TRAC #35602 (Loner does not log banner)). The logic seems to say that if isLoner is true then print the banner even if INHIBIT_DM_BANNER is true.
// log the banner
if (InternalDistributedSystem.getReconnectAttemptCounter() == 0 // avoid filling up logs during auto-reconnect
&& !isSecure && (!isLoner /* do this on a loner to fix bug 35602 */
|| !Boolean.getBoolean(InternalLocator.INHIBIT_DM_BANNER))) {
// LOG:CONFIG:
logger.info(LogMarker.CONFIG, Banner.getString(null));
}
The last block suppresses logging config if isLoner is true.
// log the config
if (logConfig) {
if (!isLoner) {
// LOG:CONFIG: changed from config to info
logger.info(LogMarker.CONFIG, LocalizedMessage.create(LocalizedStrings.InternalDistributedSystem_STARTUP_CONFIGURATIONN_0, config.toLoggerString()));
}
}
Both of the above uses of isLoner seem strange to me. Why do we suppress config and force banner if isLoner is true?
- Kirk
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47650/#review134180
-----------------------------------------------------------
On May 20, 2016, 6:48 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47650/
> -----------------------------------------------------------
>
> (Updated May 20, 2016, 6:48 p.m.)
>
>
> Review request for geode, Darrel Schneider, Hitesh Khamesra, Jianxia Chen, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1415
> https://issues.apache.org/jira/browse/GEODE-1415
>
>
> Repository: geode
>
>
> Description
> -------
>
> Banners are normally logged during creation of an InternalDistributedSystem. In DUnit tests the output is filled with these banners and they all have the same information, so they are just clutter.
>
> This change-set disables banners in non-locator VMs launched by DUnitLauncher/ProcessManager. Precheckin testing shows that the amount of output for geode-core/distributedTests alone used to be over 1gb and is now less than 500mb.
>
> Probably the only change here that requires much scrutiny is in LogWriterFactory. Darrel, I couldn't figure out what the isLoner check is there for.
>
> There are a lot of diffs in Banner.java that you can ignore - I found that it was using tabs for indentation and changed these to spaces. There are no other changes to that class.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/admin/internal/AdminDistributedSystemImpl.java 21a9ae3c14b697a94787f3236f441046ca639d55
> geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalLocator.java cf8d91ee9dc12224b8074d6c4dc8c738c00a3b0f
> geode-core/src/main/java/com/gemstone/gemfire/internal/Banner.java 4b01e54dff6f11b06f85165ca843e5a5f7b606ad
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/properties.html 1cd1b72aa8d2e312c44b435bc215885b2a312489
> geode-core/src/main/java/com/gemstone/gemfire/internal/logging/LogWriterFactory.java d9352c0c90873750402ef66ad07bec2f12935198
> geode-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java 69df873674632a8908a022d8220dcb2f329de375
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/DUnitLauncher.java 0c600ab57cad5e6443e340f1b6a160ae39b966cb
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/ProcessManager.java 5dbdfd6dee85e2de1d3c993e4ad096c5dcc1ed40
>
> Diff: https://reviews.apache.org/r/47650/diff/
>
>
> Testing
> -------
>
> precheckin had three unrelated failures that reproduce consistently without these changes.
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 47650: GEODE-1415 Disable logging of banners in
distributedTests
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47650/#review134180
-----------------------------------------------------------
Fix it, then Ship it!
geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalLocator.java (line 123)
<https://reviews.apache.org/r/47650/#comment198828>
What was the reason for changing the prefix on this system property?
I think we want to get rid of this "gemfire." prefix in geode. The old "Locator." prefix would work in both gemfire and geode.
I didn't see anything related to isLoner and I didn't see any diff for LogWriterFactory to look at.
Going back and looking at the LogWriterFactory I don't understand the !isLoner code in it.
The old ticket (35602) had to do with loners never logging a banner so I would have thought this code
would need to say something like "if isLoner then logBanner". But instead it is "if !isLoner then logBanner".
But I didn't see any problems with the changes you are making.
- Darrel Schneider
On May 20, 2016, 10 a.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47650/
> -----------------------------------------------------------
>
> (Updated May 20, 2016, 10 a.m.)
>
>
> Review request for geode, Darrel Schneider, Hitesh Khamesra, Jianxia Chen, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1415
> https://issues.apache.org/jira/browse/GEODE-1415
>
>
> Repository: geode
>
>
> Description
> -------
>
> Banners are normally logged during creation of an InternalDistributedSystem. In DUnit tests the output is filled with these banners and they all have the same information, so they are just clutter.
>
> This change-set disables banners in non-locator VMs launched by DUnitLauncher/ProcessManager. Precheckin testing shows that the amount of output for geode-core/distributedTests alone used to be over 1gb and is now less than 500mb.
>
> Probably the only change here that requires much scrutiny is in LogWriterFactory. Darrel, I couldn't figure out what the isLoner check is there for.
>
> There are a lot of diffs in Banner.java that you can ignore - I found that it was using tabs for indentation and changed these to spaces. There are no other changes to that class.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/admin/internal/AdminDistributedSystemImpl.java 21a9ae3c14b697a94787f3236f441046ca639d55
> geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalLocator.java cf8d91ee9dc12224b8074d6c4dc8c738c00a3b0f
> geode-core/src/main/java/com/gemstone/gemfire/internal/Banner.java 4b01e54dff6f11b06f85165ca843e5a5f7b606ad
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/properties.html 1cd1b72aa8d2e312c44b435bc215885b2a312489
> geode-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java d9352c0c90873750402ef66ad07bec2f12935198
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/DUnitLauncher.java 0c600ab57cad5e6443e340f1b6a160ae39b966cb
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/ProcessManager.java 5dbdfd6dee85e2de1d3c993e4ad096c5dcc1ed40
>
> Diff: https://reviews.apache.org/r/47650/diff/
>
>
> Testing
> -------
>
> precheckin had three unrelated failures that reproduce consistently without these changes.
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 47650: GEODE-1415 Disable logging of banners in
distributedTests
Posted by Bruce Schuchardt <bs...@pivotal.io>.
> On May 20, 2016, 5:07 p.m., Kirk Lund wrote:
> > I'd like to see the diff for DistributedSystemLogFileJUnitTest.java. Looks like it the diffs didn't upload properly.
I had a failure in that test that I needed to investigate and fix, so I added a pointer to the log file it created in one of the assertions.
---- a/geode-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java
+++ b/geode-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java
@@ -127,7 +127,7 @@ public class DistributedSystemLogFileJUnitTest {
// assert not empty
FileInputStream fis = new FileInputStream(logFile);
try {
- assertTrue(fis.available() > 0);
+ assertTrue("log file is empty: " + logFile.getAbsoluteFile(), fis.available() > 0);
} finally {
fis.close();
}
- Bruce
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47650/#review134163
-----------------------------------------------------------
On May 20, 2016, 5 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47650/
> -----------------------------------------------------------
>
> (Updated May 20, 2016, 5 p.m.)
>
>
> Review request for geode, Darrel Schneider, Hitesh Khamesra, Jianxia Chen, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1415
> https://issues.apache.org/jira/browse/GEODE-1415
>
>
> Repository: geode
>
>
> Description
> -------
>
> Banners are normally logged during creation of an InternalDistributedSystem. In DUnit tests the output is filled with these banners and they all have the same information, so they are just clutter.
>
> This change-set disables banners in non-locator VMs launched by DUnitLauncher/ProcessManager. Precheckin testing shows that the amount of output for geode-core/distributedTests alone used to be over 1gb and is now less than 500mb.
>
> Probably the only change here that requires much scrutiny is in LogWriterFactory. Darrel, I couldn't figure out what the isLoner check is there for.
>
> There are a lot of diffs in Banner.java that you can ignore - I found that it was using tabs for indentation and changed these to spaces. There are no other changes to that class.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/admin/internal/AdminDistributedSystemImpl.java 21a9ae3c14b697a94787f3236f441046ca639d55
> geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalLocator.java cf8d91ee9dc12224b8074d6c4dc8c738c00a3b0f
> geode-core/src/main/java/com/gemstone/gemfire/internal/Banner.java 4b01e54dff6f11b06f85165ca843e5a5f7b606ad
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/properties.html 1cd1b72aa8d2e312c44b435bc215885b2a312489
> geode-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java d9352c0c90873750402ef66ad07bec2f12935198
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/DUnitLauncher.java 0c600ab57cad5e6443e340f1b6a160ae39b966cb
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/ProcessManager.java 5dbdfd6dee85e2de1d3c993e4ad096c5dcc1ed40
>
> Diff: https://reviews.apache.org/r/47650/diff/
>
>
> Testing
> -------
>
> precheckin had three unrelated failures that reproduce consistently without these changes.
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 47650: GEODE-1415 Disable logging of banners in
distributedTests
Posted by Kirk Lund <ki...@gmail.com>.
> On May 20, 2016, 5:07 p.m., Kirk Lund wrote:
> > I'd like to see the diff for DistributedSystemLogFileJUnitTest.java. Looks like it the diffs didn't upload properly.
>
> Bruce Schuchardt wrote:
> I had a failure in that test that I needed to investigate and fix, so I added a pointer to the log file it created in one of the assertions.
>
> ---- a/geode-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java
> +++ b/geode-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java
> @@ -127,7 +127,7 @@ public class DistributedSystemLogFileJUnitTest {
> // assert not empty
> FileInputStream fis = new FileInputStream(logFile);
> try {
> - assertTrue(fis.available() > 0);
> + assertTrue("log file is empty: " + logFile.getAbsoluteFile(), fis.available() > 0);
> } finally {
> fis.close();
> }
I would go ahead and commit that change. I like adding detailed string descriptions to help diagnose a failing assertion.
- Kirk
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47650/#review134163
-----------------------------------------------------------
On May 20, 2016, 6:48 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47650/
> -----------------------------------------------------------
>
> (Updated May 20, 2016, 6:48 p.m.)
>
>
> Review request for geode, Darrel Schneider, Hitesh Khamesra, Jianxia Chen, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1415
> https://issues.apache.org/jira/browse/GEODE-1415
>
>
> Repository: geode
>
>
> Description
> -------
>
> Banners are normally logged during creation of an InternalDistributedSystem. In DUnit tests the output is filled with these banners and they all have the same information, so they are just clutter.
>
> This change-set disables banners in non-locator VMs launched by DUnitLauncher/ProcessManager. Precheckin testing shows that the amount of output for geode-core/distributedTests alone used to be over 1gb and is now less than 500mb.
>
> Probably the only change here that requires much scrutiny is in LogWriterFactory. Darrel, I couldn't figure out what the isLoner check is there for.
>
> There are a lot of diffs in Banner.java that you can ignore - I found that it was using tabs for indentation and changed these to spaces. There are no other changes to that class.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/admin/internal/AdminDistributedSystemImpl.java 21a9ae3c14b697a94787f3236f441046ca639d55
> geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalLocator.java cf8d91ee9dc12224b8074d6c4dc8c738c00a3b0f
> geode-core/src/main/java/com/gemstone/gemfire/internal/Banner.java 4b01e54dff6f11b06f85165ca843e5a5f7b606ad
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/properties.html 1cd1b72aa8d2e312c44b435bc215885b2a312489
> geode-core/src/main/java/com/gemstone/gemfire/internal/logging/LogWriterFactory.java d9352c0c90873750402ef66ad07bec2f12935198
> geode-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java 69df873674632a8908a022d8220dcb2f329de375
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/DUnitLauncher.java 0c600ab57cad5e6443e340f1b6a160ae39b966cb
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/ProcessManager.java 5dbdfd6dee85e2de1d3c993e4ad096c5dcc1ed40
>
> Diff: https://reviews.apache.org/r/47650/diff/
>
>
> Testing
> -------
>
> precheckin had three unrelated failures that reproduce consistently without these changes.
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 47650: GEODE-1415 Disable logging of banners in
distributedTests
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47650/#review134163
-----------------------------------------------------------
I'd like to see the diff for DistributedSystemLogFileJUnitTest.java. Looks like it the diffs didn't upload properly.
- Kirk Lund
On May 20, 2016, 5 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47650/
> -----------------------------------------------------------
>
> (Updated May 20, 2016, 5 p.m.)
>
>
> Review request for geode, Darrel Schneider, Hitesh Khamesra, Jianxia Chen, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1415
> https://issues.apache.org/jira/browse/GEODE-1415
>
>
> Repository: geode
>
>
> Description
> -------
>
> Banners are normally logged during creation of an InternalDistributedSystem. In DUnit tests the output is filled with these banners and they all have the same information, so they are just clutter.
>
> This change-set disables banners in non-locator VMs launched by DUnitLauncher/ProcessManager. Precheckin testing shows that the amount of output for geode-core/distributedTests alone used to be over 1gb and is now less than 500mb.
>
> Probably the only change here that requires much scrutiny is in LogWriterFactory. Darrel, I couldn't figure out what the isLoner check is there for.
>
> There are a lot of diffs in Banner.java that you can ignore - I found that it was using tabs for indentation and changed these to spaces. There are no other changes to that class.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/admin/internal/AdminDistributedSystemImpl.java 21a9ae3c14b697a94787f3236f441046ca639d55
> geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalLocator.java cf8d91ee9dc12224b8074d6c4dc8c738c00a3b0f
> geode-core/src/main/java/com/gemstone/gemfire/internal/Banner.java 4b01e54dff6f11b06f85165ca843e5a5f7b606ad
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/properties.html 1cd1b72aa8d2e312c44b435bc215885b2a312489
> geode-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java d9352c0c90873750402ef66ad07bec2f12935198
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/DUnitLauncher.java 0c600ab57cad5e6443e340f1b6a160ae39b966cb
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/standalone/ProcessManager.java 5dbdfd6dee85e2de1d3c993e4ad096c5dcc1ed40
>
> Diff: https://reviews.apache.org/r/47650/diff/
>
>
> Testing
> -------
>
> precheckin had three unrelated failures that reproduce consistently without these changes.
>
>
> Thanks,
>
> Bruce Schuchardt
>
>