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 2016/04/15 19:25:53 UTC
Review Request 46275: GEODE-1234: Provide a test hook to track
transactions scheduled to be removed
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46275/
-----------------------------------------------------------
Review request for geode, Darrel Schneider and Swapnil Bawaskar.
Bugs: GEOD-1234
https://issues.apache.org/jira/browse/GEOD-1234
Repository: geode
Description
-------
Provide a test hook to track transactions to be removed instead of waiting for default 180 seconds.
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/ClientHealthMonitor.java 93e543d
Diff: https://reviews.apache.org/r/46275/diff/
Testing
-------
Thanks,
Eric Shu
Re: Review Request 46275: GEODE-1234: Provide a test hook to track
transactions scheduled to be removed
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46275/#review131721
-----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/ClientHealthMonitor.java (line 282)
<https://reviews.apache.org/r/46275/#comment195752>
Instead of HashSet use ConcurrentHashSet so you don't have concurrency issues since multiple threads can add/remove from this.
I don't see why this is "static".
I think it makes more sense for "scheduledToBeRemovedTx" to be a non-static private instance variable that is initialized to null if the hook is not enabled and other wise initialized to an empty ConcurrentHashSet.
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/ClientHealthMonitor.java (line 314)
<https://reviews.apache.org/r/46275/#comment195753>
I would like to see this entire testHook only do the addAll and removeAll (and the allocation of the HashSet) if the hook has been enabled. Don't do this code in production. Test's that use the hook can enable it somehow (could be a system property) otherwise the set can be null.
- Darrel Schneider
On April 15, 2016, 10:25 a.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46275/
> -----------------------------------------------------------
>
> (Updated April 15, 2016, 10:25 a.m.)
>
>
> Review request for geode, Darrel Schneider and Swapnil Bawaskar.
>
>
> Bugs: GEOD-1234
> https://issues.apache.org/jira/browse/GEOD-1234
>
>
> Repository: geode
>
>
> Description
> -------
>
> Provide a test hook to track transactions to be removed instead of waiting for default 180 seconds.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/ClientHealthMonitor.java 93e543d
>
> Diff: https://reviews.apache.org/r/46275/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 46275: GEODE-1234: Provide a test hook to track
transactions scheduled to be removed
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46275/#review132303
-----------------------------------------------------------
Ship it!
Ship It!
- Darrel Schneider
On May 9, 2016, 10:56 a.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46275/
> -----------------------------------------------------------
>
> (Updated May 9, 2016, 10:56 a.m.)
>
>
> Review request for geode, Darrel Schneider and Swapnil Bawaskar.
>
>
> Bugs: GEOD-1234
> https://issues.apache.org/jira/browse/GEOD-1234
>
>
> Repository: geode
>
>
> Description
> -------
>
> Provide a test hook to track transactions to be removed instead of waiting for default 180 seconds.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/ClientHealthMonitor.java 93e543d
>
> Diff: https://reviews.apache.org/r/46275/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 46275: GEODE-1234: Provide a test hook to track
transactions scheduled to be removed
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46275/
-----------------------------------------------------------
(Updated May 9, 2016, 5:56 p.m.)
Review request for geode, Darrel Schneider and Swapnil Bawaskar.
Changes
-------
Get rid of concurrency issue during initialization.
Bugs: GEOD-1234
https://issues.apache.org/jira/browse/GEOD-1234
Repository: geode
Description
-------
Provide a test hook to track transactions to be removed instead of waiting for default 180 seconds.
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/ClientHealthMonitor.java 93e543d
Diff: https://reviews.apache.org/r/46275/diff/
Testing
-------
Thanks,
Eric Shu
Re: Review Request 46275: GEODE-1234: Provide a test hook to track
transactions scheduled to be removed
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46275/#review132102
-----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/ClientHealthMonitor.java (line 284)
<https://reviews.apache.org/r/46275/#comment196219>
I think you still have concurrency issues.
I would change this code to just have one instance variable "scheduledToBeRemovedTx". Make it "final" which gets rid of the concurrency issue I see.
Initialize it like so:
private final Set<TXId> scheduledToBeRemovedTx = Boolean.getBoolean("gemfire.trackScheduledToBeRemovedTx") ? new ConcurrentHashSet<TXId>() : null;
Then in the later code you can just say:
if (scheduledToBeRemovedTx != null) {
addAll/removeAll
}
- Darrel Schneider
On May 6, 2016, 9:36 a.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46275/
> -----------------------------------------------------------
>
> (Updated May 6, 2016, 9:36 a.m.)
>
>
> Review request for geode, Darrel Schneider and Swapnil Bawaskar.
>
>
> Bugs: GEOD-1234
> https://issues.apache.org/jira/browse/GEOD-1234
>
>
> Repository: geode
>
>
> Description
> -------
>
> Provide a test hook to track transactions to be removed instead of waiting for default 180 seconds.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/ClientHealthMonitor.java 93e543d
>
> Diff: https://reviews.apache.org/r/46275/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 46275: GEODE-1234: Provide a test hook to track
transactions scheduled to be removed
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46275/
-----------------------------------------------------------
(Updated May 6, 2016, 4:36 p.m.)
Review request for geode, Darrel Schneider and Swapnil Bawaskar.
Changes
-------
Changed to use ConcurrentHashSet.
Only initialized if system property is set.
Bugs: GEOD-1234
https://issues.apache.org/jira/browse/GEOD-1234
Repository: geode
Description
-------
Provide a test hook to track transactions to be removed instead of waiting for default 180 seconds.
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/ClientHealthMonitor.java 93e543d
Diff: https://reviews.apache.org/r/46275/diff/
Testing
-------
Thanks,
Eric Shu