You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Ken Howe <kh...@pivotal.io> on 2016/08/26 20:28:24 UTC
Review Request 51465: GEODE-1128: Cleaned up some of the colocation
logging test
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51465/
-----------------------------------------------------------
Review request for geode.
Repository: geode
Description
-------
GEODE-1128: Cleaned up some of the colocation logging tests
Diffs
-----
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java b701b70f94f0a8410ea71dd7fb77f586ba456f3f
Diff: https://reviews.apache.org/r/51465/diff/
Testing
-------
Ensure that the asyncInvocation threads terminate. Failure of a thread to terminate on one of the tests is apparently responsible for at least 1 CI failure.
Thanks,
Ken Howe
Re: Review Request 51465: GEODE-1128: Cleaned up some of the
colocation logging test
Posted by Ken Howe <kh...@pivotal.io>.
> On Aug. 26, 2016, 9:27 p.m., Darrel Schneider wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java, line 713
> > <https://reviews.apache.org/r/51465/diff/1/?file=1487003#file1487003line713>
> >
> > This is overkill for this test. Your grand-parent (JUnit4CacheTestCache) closes the cache on each jvm. I think you only need to close them early when you have an async thread stuck.
Two tests failed to call get(), getResult() or join() on all of their invokeAsync threads. One of those, testMissingColocatedParentPRWherePRConfigExists, really did have a thread that never terminated. Calls have beebn added for the threads where they were missing.
> On Aug. 26, 2016, 9:27 p.m., Darrel Schneider wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java, line 753
> > <https://reviews.apache.org/r/51465/diff/1/?file=1487003#file1487003line753>
> >
> > I think in general it would be better to have the closeCache and async0.join in a finally block after you have done the assertEquals.
> >
> > Also make clear that you expect async0 to block until closeCache is called.
This test had a thread on vm0 that never terminated. A finally block has been added to create valid region configurations on vm1 after the expceted exception we received. When the vm1 regions are recreated the operations in the thread on vm0 should complete. Calls have been added to make sure the threads terminate and the caches close.
> On Aug. 26, 2016, 9:27 p.m., Darrel Schneider wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java, line 914
> > <https://reviews.apache.org/r/51465/diff/1/?file=1487003#file1487003line914>
> >
> > Another way to unstick these async0 guys that are waiting for vm1 to do something (like create all its regions) would be to have the test create those regions and confirm that we stop logging warnings in vm1 and that vm0 completes region creation.
This particular test did not have a stuck thread. The indicated line has been remnoved as there was already an async0.get() before the assert.
- Ken
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51465/#review147025
-----------------------------------------------------------
On Aug. 26, 2016, 8:36 p.m., Ken Howe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51465/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2016, 8:36 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-1128: Cleaned up some of the colocation logging tests
>
> Ensure that the asyncInvocation threads terminate. Failure of a thread to terminate on one of the tests is apparently responsible for at least 1 CI failure.
>
>
> Diffs
> -----
>
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java b701b70f94f0a8410ea71dd7fb77f586ba456f3f
>
> Diff: https://reviews.apache.org/r/51465/diff/
>
>
> Testing
> -------
>
> precheckin in progress
>
>
> Thanks,
>
> Ken Howe
>
>
Re: Review Request 51465: GEODE-1128: Cleaned up some of the
colocation logging test
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51465/#review147025
-----------------------------------------------------------
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java (line 713)
<https://reviews.apache.org/r/51465/#comment214001>
This is overkill for this test. Your grand-parent (JUnit4CacheTestCache) closes the cache on each jvm. I think you only need to close them early when you have an async thread stuck.
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java (line 753)
<https://reviews.apache.org/r/51465/#comment213999>
I think in general it would be better to have the closeCache and async0.join in a finally block after you have done the assertEquals.
Also make clear that you expect async0 to block until closeCache is called.
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java (line 914)
<https://reviews.apache.org/r/51465/#comment214002>
Another way to unstick these async0 guys that are waiting for vm1 to do something (like create all its regions) would be to have the test create those regions and confirm that we stop logging warnings in vm1 and that vm0 completes region creation.
- Darrel Schneider
On Aug. 26, 2016, 1:36 p.m., Ken Howe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51465/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2016, 1:36 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-1128: Cleaned up some of the colocation logging tests
>
> Ensure that the asyncInvocation threads terminate. Failure of a thread to terminate on one of the tests is apparently responsible for at least 1 CI failure.
>
>
> Diffs
> -----
>
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java b701b70f94f0a8410ea71dd7fb77f586ba456f3f
>
> Diff: https://reviews.apache.org/r/51465/diff/
>
>
> Testing
> -------
>
> precheckin in progress
>
>
> Thanks,
>
> Ken Howe
>
>
Re: Review Request 51465: GEODE-1128: Cleaned up some of the
colocation logging test
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51465/#review147455
-----------------------------------------------------------
Ship it!
Ship It!
- Darrel Schneider
On Aug. 31, 2016, 9:53 a.m., Ken Howe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51465/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2016, 9:53 a.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-1128: Cleaned up some of the colocation logging tests
>
> Ensure that the asyncInvocation threads terminate. Failure of a thread to terminate on one of the tests is apparently responsible for at least 1 CI failure.
>
>
> Diffs
> -----
>
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java b701b70
>
> Diff: https://reviews.apache.org/r/51465/diff/
>
>
> Testing
> -------
>
> precheckin in progress
>
>
> Thanks,
>
> Ken Howe
>
>
Re: Review Request 51465: GEODE-1128: Cleaned up some of the
colocation logging test
Posted by Ken Howe <kh...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51465/
-----------------------------------------------------------
(Updated Aug. 31, 2016, 4:53 p.m.)
Review request for geode, anilkumar gingade, Darrel Schneider, and Kirk Lund.
Changes
-------
Updated from previous review. Precheckin was run with no failures
Repository: geode
Description
-------
GEODE-1128: Cleaned up some of the colocation logging tests
Ensure that the asyncInvocation threads terminate. Failure of a thread to terminate on one of the tests is apparently responsible for at least 1 CI failure.
Diffs (updated)
-----
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java b701b70
Diff: https://reviews.apache.org/r/51465/diff/
Testing
-------
precheckin in progress
Thanks,
Ken Howe
Re: Review Request 51465: GEODE-1128: Cleaned up some of the
colocation logging test
Posted by Ken Howe <kh...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51465/
-----------------------------------------------------------
(Updated Aug. 26, 2016, 8:36 p.m.)
Review request for geode, anilkumar gingade, Darrel Schneider, and Kirk Lund.
Repository: geode
Description (updated)
-------
GEODE-1128: Cleaned up some of the colocation logging tests
Ensure that the asyncInvocation threads terminate. Failure of a thread to terminate on one of the tests is apparently responsible for at least 1 CI failure.
Diffs
-----
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java b701b70f94f0a8410ea71dd7fb77f586ba456f3f
Diff: https://reviews.apache.org/r/51465/diff/
Testing (updated)
-------
precheckin in progress
Thanks,
Ken Howe