> On Nov. 17, 2015, 7:37 p.m., Darrel Schneider wrote: > > gemfire-core/src/test/java/com/gemstone/gemfire/management/DistributedSystemDUnitTest.java, > > line 179 > > <https://reviews.apache.org/r/40369/diff/1/?file=1127622#file1127622line179> > > > > To test your fix don't you need to set the alert level to warning and > > then immediately to severe and then check and make sure everyone has it set > > to severe and not warning? > > > > Since the setAlertLevel message sleeps for 10 seconds and I you need to > > refactor this test a bit to have this every happen. I could be wrong but it > > doesn't seem like this test would have failed before you fixed the message > > to be serial. > > Also it doesn't see like you want to call warnLevelAlert and > > severeLevelAlert between the calls of setAlertLevel since these also take > > some time (they generate log messages).
I'll add a different test for that. This fix addresses the problem of async messages arriving out of order. The test was failing intermittently because of that. > On Nov. 17, 2015, 7:37 p.m., Darrel Schneider wrote: > > gemfire-core/src/test/java/com/gemstone/gemfire/management/DistributedSystemDUnitTest.java, > > line 190 > > <https://reviews.apache.org/r/40369/diff/1/?file=1127622#file1127622line190> > > > > Something I don't see in this diff is when this line gets called: > > setAlertLevel(managingNode, > > AlertDetails.getAlertLevelAsString(Alert.OFF)); > > > > It seems like this test should have this in a finally block to make > > sure we attempt to turn alerting back off. I don't think alerting is ever off - the default is for the level to be set at SEVERE. - Jens ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40369/#review106920 ----------------------------------------------------------- On Nov. 19, 2015, 8:59 p.m., Jens Deppe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40369/ > ----------------------------------------------------------- > > (Updated Nov. 19, 2015, 8:59 p.m.) > > > Review request for geode and Dan Smith. > > > Repository: geode > > > Description > ------- > > - Updating the alert level is changed from a pooled message to a serial > message. This ensures that consecutive alert level changes are > processed in the correct order. > > Also reworked the test so that we're not trying to set the same alert level > twice in a row otherwise it becomes difficult to know where the messages have > been received. > > > Diffs > ----- > > > gemfire-core/src/main/java/com/gemstone/gemfire/internal/admin/remote/AlertLevelChangeMessage.java > 7e6c0a4ef08cb53f16d7b024778625fbeb976cca > > gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/log4j/AlertAppender.java > bd1ad9225a5ea1c761e92a52d4da3dab72ce9ff3 > > gemfire-core/src/test/java/com/gemstone/gemfire/management/DistributedSystemDUnitTest.java > 193dd1222e235311413ccbb89a00ecf43e657de1 > > Diff: https://reviews.apache.org/r/40369/diff/ > > > Testing > ------- > > Ran DistributedSystemDUnitTest > > > Thanks, > > Jens Deppe > >
