----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48738/#review137852 -----------------------------------------------------------
Fix it, then Ship it! Looks good, especially all of your detailed testing! I have a few comments/suggestions, below. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/AbstractGatewaySender.java (line 813) <https://reviews.apache.org/r/48738/#comment203041> You could probably get rid of the nested if conditions here: // Check if its AEQ and is configured to forward expiration destroy events. if (event.getOperation().isExpiration() && this.isAsyncEventQueue() && this.isForwardExpirationDestroy()) { return true; } if (event.getOperation().isLocal() || event.getOperation().isExpiration()) { return false; } geode-core/src/test/java/com/gemstone/gemfire/cache/asyncqueue/AsyncEventQueueEvictionAndExpirationJUnitTest.java (line 240) <https://reviews.apache.org/r/48738/#comment203047> This is one big test method with a lot of boolean flags to check different conditions. Maybe this should be broken into smaller, simpler test methods? For example checkForInvalidOp could be it's own method, rather than a flag. geode-core/src/test/java/com/gemstone/gemfire/cache/asyncqueue/AsyncEventQueueEvictionAndExpirationJUnitTest.java (line 262) <https://reviews.apache.org/r/48738/#comment203043> I think it's better to put assertions into awaitility, like this Awaitility...until(() -> assertEquals(1, r.size()) Although I guess in this case you are using >=, rather than ==. But why is that? Can't you make an assertion about exactly what size the region should be? geode-core/src/test/java/com/gemstone/gemfire/cache/asyncqueue/AsyncEventQueueEvictionAndExpirationJUnitTest.java (line 283) <https://reviews.apache.org/r/48738/#comment203044> If you use an assertion in the below awaility clause, you woudln't need a System.out.println. geode-core/src/test/java/com/gemstone/gemfire/cache/asyncqueue/AsyncEventQueueEvictionAndExpirationJUnitTest.java (line 372) <https://reviews.apache.org/r/48738/#comment203045> Remove commented out printlns - Dan Smith On June 15, 2016, 5:42 p.m., anilkumar gingade wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48738/ > ----------------------------------------------------------- > > (Updated June 15, 2016, 5:42 p.m.) > > > Review request for geode, anilkumar gingade, Barry Oglesby, Darrel Schneider, > Jason Huynh, Jens Deppe, William Markito, nabarun nag, Dan Smith, and > xiaojian zhou. > > > Repository: geode > > > Description > ------- > > GEODE-1209 was proposed to add new attribute ignoreEvictionAndExpiration to > AsyncEventQueue...As mentioned in the ticket due to product issue a new > proposal was made to change the functionality, to only forward > expiration-destroy operation. > > The changes made here replaces ignoreEvictionAndExpiration attribute to > forwardExiprationDestroy. > > > Diffs > ----- > > > geode-assembly/src/test/java/com/gemstone/gemfire/management/internal/configuration/SharedConfigurationEndToEndDUnitTest.java > 93cb31a > > geode-core/src/main/java/com/gemstone/gemfire/cache/asyncqueue/AsyncEventQueue.java > edf887b > > geode-core/src/main/java/com/gemstone/gemfire/cache/asyncqueue/AsyncEventQueueFactory.java > 6294dfe > > geode-core/src/main/java/com/gemstone/gemfire/cache/asyncqueue/internal/AsyncEventQueueFactoryImpl.java > 1ec3ba0 > > geode-core/src/main/java/com/gemstone/gemfire/cache/asyncqueue/internal/AsyncEventQueueImpl.java > 5a0b370 > geode-core/src/main/java/com/gemstone/gemfire/cache/wan/GatewaySender.java > d559a1a > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/AbstractGatewaySender.java > 7e2a0af > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/GatewaySenderAttributes.java > 163943f > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/AsyncEventQueueCreation.java > e55ec3f > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/CacheXml.java > 4ee3585 > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/CacheXmlGenerator.java > 17076db > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/CacheXmlParser.java > 76ab0f9 > > geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/commands/QueueCommands.java > d84959f > > geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/functions/AsyncEventQueueFunctionArgs.java > 2066628 > > geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/functions/CreateAsyncEventQueueFunction.java > 32e8f83 > > geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/i18n/CliStrings.java > cc80de8 > > geode-core/src/main/java/com/gemstone/gemfire/management/internal/web/controllers/QueueCommandsController.java > 9367612 > > geode-core/src/main/resources/META-INF/schemas/geode.apache.org/schema/cache/cache-1.0.xsd > 688ff1f > > geode-core/src/test/java/com/gemstone/gemfire/cache/asyncqueue/AsyncEventQueueEvictionAndExpirationJUnitTest.java > 4c5944b > > geode-core/src/test/java/com/gemstone/gemfire/cache30/CacheXmlGeode10DUnitTest.java > be4c657 > > geode-core/src/test/java/com/gemstone/gemfire/management/internal/cli/commands/QueueCommandsDUnitTest.java > c1877fe > > geode-core/src/test/resources/com/gemstone/gemfire/management/internal/cli/commands/golden-help-offline.properties > 3c0d388 > > Diff: https://reviews.apache.org/r/48738/diff/ > > > Testing > ------- > > - Test for ignoreEvictionAndExpiration was changeg to test > forwardExpirationDestroy > - SharedConfigurationEndToEndDUnitTest > - CacheXmlGeode10DUnitTest > - QueueCommandsDUnitTest > > > Thanks, > > anilkumar gingade > >
