You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Darrel Schneider <ds...@pivotal.io> on 2015/08/10 23:28:56 UTC
Review Request 37321: GEODE-138: remove race condition from
testEventDelivery
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37321/
-----------------------------------------------------------
Review request for geode and Kirk Lund.
Bugs: GEODE-138
https://issues.apache.org/jira/browse/GEODE-138
Repository: geode
Description
-------
The race due to the test expecting local event delivery to be synchronous
but the implementation of off-heap resource manager events being async.
The test has been changed to always use a wait criteria when waiting
for a memory event to arrive.
Diffs
-----
gemfire-core/src/test/java/com/gemstone/gemfire/cache/management/MemoryThresholdsOffHeapDUnitTest.java 0b704a6955e641e249c00fc0942d4bca214ebbaf
Diff: https://reviews.apache.org/r/37321/diff/
Testing
-------
Ran MemoryThresholdsOffHeapDUnitTest (it was the only thing modified by this fix).
Thanks,
Darrel Schneider
Re: Review Request 37321: GEODE-138: remove race condition from
testEventDelivery
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37321/#review94825
-----------------------------------------------------------
Ship it!
Ship It!
- Kirk Lund
On Aug. 10, 2015, 10:02 p.m., Darrel Schneider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37321/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2015, 10:02 p.m.)
>
>
> Review request for geode and Kirk Lund.
>
>
> Bugs: GEODE-138
> https://issues.apache.org/jira/browse/GEODE-138
>
>
> Repository: geode
>
>
> Description
> -------
>
> The race due to the test expecting local event delivery to be synchronous
> but the implementation of off-heap resource manager events being async.
> The test has been changed to always use a wait criteria when waiting
> for a memory event to arrive.
>
>
> Diffs
> -----
>
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/management/MemoryThresholdsOffHeapDUnitTest.java 0b704a6955e641e249c00fc0942d4bca214ebbaf
>
> Diff: https://reviews.apache.org/r/37321/diff/
>
>
> Testing
> -------
>
> Ran MemoryThresholdsOffHeapDUnitTest (it was the only thing modified by this fix).
>
>
> Thanks,
>
> Darrel Schneider
>
>
Re: Review Request 37321: GEODE-138: remove race condition from
testEventDelivery
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37321/
-----------------------------------------------------------
(Updated Aug. 10, 2015, 3:02 p.m.)
Review request for geode and Kirk Lund.
Changes
-------
Based on review feedback change to callers to pass "true" as the useWaitCriterion
Bugs: GEODE-138
https://issues.apache.org/jira/browse/GEODE-138
Repository: geode
Description
-------
The race due to the test expecting local event delivery to be synchronous
but the implementation of off-heap resource manager events being async.
The test has been changed to always use a wait criteria when waiting
for a memory event to arrive.
Diffs (updated)
-----
gemfire-core/src/test/java/com/gemstone/gemfire/cache/management/MemoryThresholdsOffHeapDUnitTest.java 0b704a6955e641e249c00fc0942d4bca214ebbaf
Diff: https://reviews.apache.org/r/37321/diff/
Testing
-------
Ran MemoryThresholdsOffHeapDUnitTest (it was the only thing modified by this fix).
Thanks,
Darrel Schneider
Re: Review Request 37321: GEODE-138: remove race condition from
testEventDelivery
Posted by Darrel Schneider <ds...@pivotal.io>.
> On Aug. 10, 2015, 2:36 p.m., Kirk Lund wrote:
> > gemfire-core/src/test/java/com/gemstone/gemfire/cache/management/MemoryThresholdsOffHeapDUnitTest.java, line 1587
> > <https://reviews.apache.org/r/37321/diff/1/?file=1036658#file1036658line1587>
> >
> > Did you want to hardcode to true or add a line to assert the argument is true?
> >
> > Or maybe change the parameter to non-final and have the 1st line of this method change it's value to true?
Originally I thought I would just change the callers to pass true in. In the future their might be a valid case for the event being sync in which case this method could then be used with a parameter of false. Since the parameter is accessed from a wait criteria it has to be a final. And it looked pretty wierd to make it no final; and then have a local final var that takes the param and just makes it true.
I'll go ahead and remove my changes to the param and just change all the current callers to set the param to true.
- Darrel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37321/#review94801
-----------------------------------------------------------
On Aug. 10, 2015, 2:28 p.m., Darrel Schneider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37321/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2015, 2:28 p.m.)
>
>
> Review request for geode and Kirk Lund.
>
>
> Bugs: GEODE-138
> https://issues.apache.org/jira/browse/GEODE-138
>
>
> Repository: geode
>
>
> Description
> -------
>
> The race due to the test expecting local event delivery to be synchronous
> but the implementation of off-heap resource manager events being async.
> The test has been changed to always use a wait criteria when waiting
> for a memory event to arrive.
>
>
> Diffs
> -----
>
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/management/MemoryThresholdsOffHeapDUnitTest.java 0b704a6955e641e249c00fc0942d4bca214ebbaf
>
> Diff: https://reviews.apache.org/r/37321/diff/
>
>
> Testing
> -------
>
> Ran MemoryThresholdsOffHeapDUnitTest (it was the only thing modified by this fix).
>
>
> Thanks,
>
> Darrel Schneider
>
>
Re: Review Request 37321: GEODE-138: remove race condition from
testEventDelivery
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37321/#review94801
-----------------------------------------------------------
gemfire-core/src/test/java/com/gemstone/gemfire/cache/management/MemoryThresholdsOffHeapDUnitTest.java (line 1587)
<https://reviews.apache.org/r/37321/#comment149422>
Did you want to hardcode to true or add a line to assert the argument is true?
Or maybe change the parameter to non-final and have the 1st line of this method change it's value to true?
- Kirk Lund
On Aug. 10, 2015, 9:28 p.m., Darrel Schneider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37321/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2015, 9:28 p.m.)
>
>
> Review request for geode and Kirk Lund.
>
>
> Bugs: GEODE-138
> https://issues.apache.org/jira/browse/GEODE-138
>
>
> Repository: geode
>
>
> Description
> -------
>
> The race due to the test expecting local event delivery to be synchronous
> but the implementation of off-heap resource manager events being async.
> The test has been changed to always use a wait criteria when waiting
> for a memory event to arrive.
>
>
> Diffs
> -----
>
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/management/MemoryThresholdsOffHeapDUnitTest.java 0b704a6955e641e249c00fc0942d4bca214ebbaf
>
> Diff: https://reviews.apache.org/r/37321/diff/
>
>
> Testing
> -------
>
> Ran MemoryThresholdsOffHeapDUnitTest (it was the only thing modified by this fix).
>
>
> Thanks,
>
> Darrel Schneider
>
>