You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by anilkumar gingade <ag...@pivotal.io> on 2016/06/15 17:42:55 UTC
Re: Review Request 48738: GEODE-1209 Changes to
ignoreEvictionAndExpiration
AEQ attribute based on the new proposal (changing
ignoreEvictionAndExpiration to forwardExpirationDestroy)
-----------------------------------------------------------
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.
Summary (updated)
-----------------
GEODE-1209 Changes to ignoreEvictionAndExpiration AEQ attribute based on the new proposal (changing ignoreEvictionAndExpiration to forwardExpirationDestroy)
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
Re: Review Request 48738: GEODE-1209 Changes to
ignoreEvictionAndExpiration
AEQ attribute based on the new proposal (changing
ignoreEvictionAndExpiration to forwardExpirationDestroy)
Posted by anilkumar gingade <ag...@pivotal.io>.
> On June 15, 2016, 9:34 p.m., Dan Smith wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/AbstractGatewaySender.java, line 815
> > <https://reviews.apache.org/r/48738/diff/1/?file=1420204#file1420204line815>
> >
> > 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;
> > }
The reason I added is to have eviction/expiration in the same context...I can break it down...
> On June 15, 2016, 9:34 p.m., Dan Smith wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/cache/asyncqueue/AsyncEventQueueEvictionAndExpirationJUnitTest.java, line 283
> > <https://reviews.apache.org/r/48738/diff/1/?file=1420216#file1420216line283>
> >
> > If you use an assertion in the below awaility clause, you woudln't need a System.out.println.
This is for debugging...Forgot to remove...
> On June 15, 2016, 9:34 p.m., Dan Smith wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/cache/asyncqueue/AsyncEventQueueEvictionAndExpirationJUnitTest.java, line 240
> > <https://reviews.apache.org/r/48738/diff/1/?file=1420216#file1420216line240>
> >
> > 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.
I am breaking it down...All the validations are not required in all methods...I am moving validations within test methods.
> On June 15, 2016, 9:34 p.m., Dan Smith wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/cache/asyncqueue/AsyncEventQueueEvictionAndExpirationJUnitTest.java, line 262
> > <https://reviews.apache.org/r/48738/diff/1/?file=1420216#file1420216line262>
> >
> > 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?
Good point...Done.
- anilkumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48738/#review137852
-----------------------------------------------------------
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
>
>
Re: Review Request 48738: GEODE-1209 Changes to
ignoreEvictionAndExpiration
AEQ attribute based on the new proposal (changing
ignoreEvictionAndExpiration to forwardExpirationDestroy)
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
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
>
>