You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Eric Shu <es...@pivotal.io> on 2017/08/24 20:13:33 UTC
Review Request 61895: GEDOE-3516: TXManagerImpl.tryResume call may add
itself again into the waiting thread queue
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61895/
-----------------------------------------------------------
Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
Bugs: GEODE-3516
https://issues.apache.org/jira/browse/GEODE-3516
Repository: geode
Description
-------
Remove the thread from waiting thread queue after successfully resumed the transaction
Diffs
-----
geode-core/src/main/java/org/apache/geode/internal/cache/TXManagerImpl.java a0a4d7c
Diff: https://reviews.apache.org/r/61895/diff/1/
Testing
-------
precheckin.
Thanks,
Eric Shu
Re: Review Request 61895: GEDOE-3516: TXManagerImpl.tryResume call
may add itself again into the waiting thread queue
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61895/#review183871
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/TXManagerImpl.java
Line 1302 (original), 1302 (patched)
<https://reviews.apache.org/r/61895/#comment259934>
Lets not make this package protection for unit test access. Instead keep it private and add an accessor for the unit tests. Also try to not expose the whole map to unit tests. For example in this case the unit test just needs to check that no threads are still waiting for a particular txid.
geode-core/src/main/java/org/apache/geode/internal/cache/TXManagerImpl.java
Lines 1346 (patched)
<https://reviews.apache.org/r/61895/#comment259935>
Instead of fixing this in the finally block we decided it would be best to keep this thread from adding itself multiple times to the queue. Also we think other race conditions exist in which the while loop that calls tryResume does not recheck if the tx still exists. Also it seems like a race exists in which suspend unparks a tryResume call that ends up not resuming because of the timeout but does not poll the queue again and unpark the next waiter.
- Darrel Schneider
On Aug. 25, 2017, 9:53 a.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61895/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2017, 9:53 a.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
>
>
> Bugs: GEODE-3516
> https://issues.apache.org/jira/browse/GEODE-3516
>
>
> Repository: geode
>
>
> Description
> -------
>
> Remove the thread from waiting thread queue after successfully resumed the transaction
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/TXManagerImpl.java a0a4d7c
> geode-core/src/test/java/org/apache/geode/internal/cache/TXManagerImplJUnitTest.java a2c1e70
>
>
> Diff: https://reviews.apache.org/r/61895/diff/2/
>
>
> Testing
> -------
>
> precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 61895: GEDOE-3516: TXManagerImpl.tryResume call
may add itself again into the waiting thread queue
Posted by Nick Reich <nr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61895/#review184710
-----------------------------------------------------------
Ship it!
Ship It!
- Nick Reich
On Sept. 6, 2017, 6:42 p.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61895/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 6:42 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
>
>
> Bugs: GEODE-3516
> https://issues.apache.org/jira/browse/GEODE-3516
>
>
> Repository: geode
>
>
> Description
> -------
>
> Remove the thread from waiting thread queue after successfully resumed the transaction
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/TXManagerImpl.java a0a4d7c
> geode-core/src/test/java/org/apache/geode/internal/cache/TXManagerImplJUnitTest.java a2c1e70
>
>
> Diff: https://reviews.apache.org/r/61895/diff/3/
>
>
> Testing
> -------
>
> precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 61895: GEDOE-3516: TXManagerImpl.tryResume call
may add itself again into the waiting thread queue
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61895/#review184709
-----------------------------------------------------------
Ship it!
Ship It!
- Darrel Schneider
On Sept. 6, 2017, 11:42 a.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61895/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 11:42 a.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
>
>
> Bugs: GEODE-3516
> https://issues.apache.org/jira/browse/GEODE-3516
>
>
> Repository: geode
>
>
> Description
> -------
>
> Remove the thread from waiting thread queue after successfully resumed the transaction
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/TXManagerImpl.java a0a4d7c
> geode-core/src/test/java/org/apache/geode/internal/cache/TXManagerImplJUnitTest.java a2c1e70
>
>
> Diff: https://reviews.apache.org/r/61895/diff/3/
>
>
> Testing
> -------
>
> precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 61895: GEDOE-3516: TXManagerImpl.tryResume call
may add itself again into the waiting thread queue
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61895/
-----------------------------------------------------------
(Updated Sept. 6, 2017, 6:42 p.m.)
Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
Changes
-------
Fix review comments -- port the changes from Darrel.
Bugs: GEODE-3516
https://issues.apache.org/jira/browse/GEODE-3516
Repository: geode
Description
-------
Remove the thread from waiting thread queue after successfully resumed the transaction
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/internal/cache/TXManagerImpl.java a0a4d7c
geode-core/src/test/java/org/apache/geode/internal/cache/TXManagerImplJUnitTest.java a2c1e70
Diff: https://reviews.apache.org/r/61895/diff/3/
Changes: https://reviews.apache.org/r/61895/diff/2-3/
Testing
-------
precheckin.
Thanks,
Eric Shu
Re: Review Request 61895: GEDOE-3516: TXManagerImpl.tryResume call
may add itself again into the waiting thread queue
Posted by Nick Reich <nr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61895/#review183945
-----------------------------------------------------------
geode-core/src/test/java/org/apache/geode/internal/cache/TXManagerImplJUnitTest.java
Lines 380 (patched)
<https://reviews.apache.org/r/61895/#comment259991>
What does this Thread.sleep() do, if the next line is awaiting a latch?
geode-core/src/test/java/org/apache/geode/internal/cache/TXManagerImplJUnitTest.java
Lines 384 (patched)
<https://reviews.apache.org/r/61895/#comment259995>
Is spyMgr.suspend() here being attempted to occur between when the two threads start and before they complete? If so, is that guaranteed to occur?
- Nick Reich
On Aug. 25, 2017, 4:53 p.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61895/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2017, 4:53 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
>
>
> Bugs: GEODE-3516
> https://issues.apache.org/jira/browse/GEODE-3516
>
>
> Repository: geode
>
>
> Description
> -------
>
> Remove the thread from waiting thread queue after successfully resumed the transaction
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/TXManagerImpl.java a0a4d7c
> geode-core/src/test/java/org/apache/geode/internal/cache/TXManagerImplJUnitTest.java a2c1e70
>
>
> Diff: https://reviews.apache.org/r/61895/diff/2/
>
>
> Testing
> -------
>
> precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 61895: GEDOE-3516: TXManagerImpl.tryResume call
may add itself again into the waiting thread queue
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61895/#review184582
-----------------------------------------------------------
geode-core/src/test/java/org/apache/geode/internal/cache/TXManagerImplJUnitTest.java
Lines 380 (patched)
<https://reviews.apache.org/r/61895/#comment260750>
To make sure the thread wait for 300 ms before calling suspend method.
geode-core/src/test/java/org/apache/geode/internal/cache/TXManagerImplJUnitTest.java
Lines 384 (patched)
<https://reviews.apache.org/r/61895/#comment260751>
The other two threads calls spyMgr.tryResume, and that method call will continue to park untill another thread calls suspend to unpark a waiting thread. Yes, it is guaranteed.
- Eric Shu
On Aug. 25, 2017, 4:53 p.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61895/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2017, 4:53 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
>
>
> Bugs: GEODE-3516
> https://issues.apache.org/jira/browse/GEODE-3516
>
>
> Repository: geode
>
>
> Description
> -------
>
> Remove the thread from waiting thread queue after successfully resumed the transaction
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/TXManagerImpl.java a0a4d7c
> geode-core/src/test/java/org/apache/geode/internal/cache/TXManagerImplJUnitTest.java a2c1e70
>
>
> Diff: https://reviews.apache.org/r/61895/diff/2/
>
>
> Testing
> -------
>
> precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 61895: GEDOE-3516: TXManagerImpl.tryResume call
may add itself again into the waiting thread queue
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61895/
-----------------------------------------------------------
(Updated Aug. 25, 2017, 4:53 p.m.)
Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
Changes
-------
Add a unit test for the fix
Bugs: GEODE-3516
https://issues.apache.org/jira/browse/GEODE-3516
Repository: geode
Description
-------
Remove the thread from waiting thread queue after successfully resumed the transaction
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/internal/cache/TXManagerImpl.java a0a4d7c
geode-core/src/test/java/org/apache/geode/internal/cache/TXManagerImplJUnitTest.java a2c1e70
Diff: https://reviews.apache.org/r/61895/diff/2/
Changes: https://reviews.apache.org/r/61895/diff/1-2/
Testing
-------
precheckin.
Thanks,
Eric Shu
Re: Review Request 61895: GEDOE-3516: TXManagerImpl.tryResume call
may add itself again into the waiting thread queue
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61895/#review183799
-----------------------------------------------------------
Would it be possible to add a unit test for this fix?
- Darrel Schneider
On Aug. 24, 2017, 1:13 p.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61895/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2017, 1:13 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
>
>
> Bugs: GEODE-3516
> https://issues.apache.org/jira/browse/GEODE-3516
>
>
> Repository: geode
>
>
> Description
> -------
>
> Remove the thread from waiting thread queue after successfully resumed the transaction
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/TXManagerImpl.java a0a4d7c
>
>
> Diff: https://reviews.apache.org/r/61895/diff/1/
>
>
> Testing
> -------
>
> precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>