----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46243/#review131925 -----------------------------------------------------------
Nice job! I had a few comments, see below. It also might be nice to try to start adding some real unit tests for some of these core classes. Would it be possible to add a unit test for the distribute method (Which calls your new checkForDistribution code)? geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java (line 1151) <https://reviews.apache.org/r/46243/#comment195966> We're not generating event ids if there is an async event queue? What is this code for? It seems like this check ought to be extracted into it's own method. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java (line 6645) <https://reviews.apache.org/r/46243/#comment195967> Are you sure there aren't another internal regions that ought to notify the gateway? Why did you add this check? THis might read cleaner if you had a separate if for each check with a comment explaining why it's skipped. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java (line 6650) <https://reviews.apache.org/r/46243/#comment195968> Remove this log message. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/AbstractGatewaySender.java (line 808) <https://reviews.apache.org/r/46243/#comment195970> Remove this log message. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/AbstractGatewaySender.java (line 816) <https://reviews.apache.org/r/46243/#comment195971> I wonder if there are use cases where we would want to pass these events to the WAN? Is there a reason we need is AEQ check here? geode-core/src/test/java/com/gemstone/gemfire/cache/asyncqueue/AsyncEventQueueEvictionAndExpirationJUnitTest.java (line 63) <https://reviews.apache.org/r/46243/#comment195972> This is not a unit test if it's creating a cache. Should be marked as an integration test. - Dan Smith On May 4, 2016, 1:09 a.m., anilkumar gingade wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46243/ > ----------------------------------------------------------- > > (Updated May 4, 2016, 1:09 a.m.) > > > Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, > nabarun nag, Dan Smith, and xiaojian zhou. > > > Repository: geode > > > Description > ------- > > GEODE-1209: Added new attribute to forward eviction/expiration to AEQ. Tested > with manual testing. > > Sending the changes for early feedback on attribute/method names... > > Need to add support for xml, gfsh...And new tests... > > > Diffs > ----- > > > geode-core/src/main/java/com/gemstone/gemfire/cache/asyncqueue/AsyncEventQueue.java > a2b8b0f62333082e8eaf87354c0a2700f7428608 > > geode-core/src/main/java/com/gemstone/gemfire/cache/asyncqueue/AsyncEventQueueFactory.java > 3e30b3847c2cd1c4fc72f342f0898a1e56cbce2e > > geode-core/src/main/java/com/gemstone/gemfire/cache/asyncqueue/internal/AsyncEventQueueFactoryImpl.java > 312e880251cb5fdcfb79b81d888ec5a85649d9da > > geode-core/src/main/java/com/gemstone/gemfire/cache/asyncqueue/internal/AsyncEventQueueImpl.java > 6b3eb4a827add8c8551bcd095eba0ab69a74dbd4 > geode-core/src/main/java/com/gemstone/gemfire/cache/wan/GatewaySender.java > c5b5d3aca14000e82b25e0eb156f789f7e1fc753 > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java > 3ad294c10463b87ed9685b4c7f767523d0a52335 > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/AbstractGatewaySender.java > fe09d031c7e326b4eb0cc3f63700c282b734b842 > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/GatewaySenderAttributes.java > 1cef940f8ebacb22cd3b08513ea3083ecd1df909 > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/AsyncEventQueueCreation.java > 001566504db6eb2828cdcc3eba824e90d1df70e3 > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/CacheXml.java > aa7d49a654fcc0c7d2419546f4b4b5e1c4444271 > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/CacheXmlGenerator.java > ea3c975267b3b05cd1e9350ca464aba70e459113 > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/CacheXmlParser.java > f344938350a1043655f27f75eb7a9cbf9246ac30 > > geode-core/src/main/resources/META-INF/schemas/geode.apache.org/schema/cache/cache-1.0.xsd > cc6d189e7fb4ed05f7bf35a9e43bafc8dc53056f > > geode-core/src/test/java/com/gemstone/gemfire/cache/asyncqueue/AsyncEventQueueEvictionAndExpirationJUnitTest.java > PRE-CREATION > > geode-core/src/test/java/com/gemstone/gemfire/cache30/CacheXmlGeode10DUnitTest.java > 57e3a13b320e302f4be4ff40aa976eabd929435a > > Diff: https://reviews.apache.org/r/46243/diff/ > > > Testing > ------- > > Did manual testing with AEQ with and without eviction and expiration. > > > Thanks, > > anilkumar gingade > >
