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/07/26 17:37:36 UTC
Review Request 61143: GEODE-3310 Set target node in TXStateProxy
during TXFailover if necessary
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61143/
-----------------------------------------------------------
Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
Bugs: GEODE-3310
https://issues.apache.org/jira/browse/GEODE-3310
Repository: geode
Description
-------
Set target node in TXStateProxy during TXFailover if necessary
Diffs
-----
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommand.java 6bd00c0
geode-core/src/test/java/org/apache/geode/internal/cache/execute/MyTransactionFunction.java 0970836
geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java 7a56644
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommandTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/61143/diff/1/
Testing
-------
Ongoing precheckin.
Thanks,
Eric Shu
Re: Review Request 61143: GEODE-3310 Set target node in TXStateProxy
during TXFailover if necessary
Posted by Nick Reich <nr...@pivotal.io>.
> On July 26, 2017, 8:11 p.m., Nick Reich wrote:
> >
All my comments are nits and only suggestions / thoughts and not barriers to accepting this review and shipping it.
- Nick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61143/#review181480
-----------------------------------------------------------
On July 26, 2017, 5:37 p.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61143/
> -----------------------------------------------------------
>
> (Updated July 26, 2017, 5:37 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
>
>
> Bugs: GEODE-3310
> https://issues.apache.org/jira/browse/GEODE-3310
>
>
> Repository: geode
>
>
> Description
> -------
>
> Set target node in TXStateProxy during TXFailover if necessary
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommand.java 6bd00c0
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/MyTransactionFunction.java 0970836
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java 7a56644
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommandTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/61143/diff/1/
>
>
> Testing
> -------
>
> Ongoing precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 61143: GEODE-3310 Set target node in TXStateProxy
during TXFailover if necessary
Posted by Nick Reich <nr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61143/#review181480
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommand.java
Line 64 (original), 64 (patched)
<https://reviews.apache.org/r/61143/#comment257091>
This does not really get a TXId, it creates one. Maybe getNewTXId() or createTXId() would be better names?
geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
Lines 178 (patched)
<https://reviews.apache.org/r/61143/#comment257093>
With "e" or "ex" being the most common names for Exceptions in java catch blocks and "exp" being an abbreviation for "expression", maybe Execution exec and Exception e would be better variable names?
geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
Lines 180 (patched)
<https://reviews.apache.org/r/61143/#comment257094>
assertFalse("Expected exception was not thrown", isFirstFunc)?
geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
Lines 184 (patched)
<https://reviews.apache.org/r/61143/#comment257096>
Possible alternative:
catch (TransactionException e) {
assertFalse("Unexpected exception was thrown", isFirstFunction)
assertTrue(e.getMessage().startsWith("Function execution is not colocated with transaction.")
}
Letting unexpected exceptions be thrown from the test will result in them being logged and simplify the logic of the test (and not require manually logging that information as well). You could arguably remove the check for the contents of the exception message, as that makes the test brittle to changes that do not effect behavior (i.e. error message text changes). If the type of exception thrown is not sufficient, does this suggest we need a new exception that inherits from TransactionException?
geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
Line 149 (original), 223 (patched)
<https://reviews.apache.org/r/61143/#comment257097>
Replacing all these anonymous classes with lambdas really helps clean up the code: I am glad we are doing more of this.
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommandTest.java
Lines 64 (patched)
<https://reviews.apache.org/r/61143/#comment257098>
Ouch! That is a massive amount of mocking and even power mocking. I do not envy you in trying to cobble that together. Is this easier/possible to do in an integration test (dunit or otherwise)? Whenever I create tests, the more mocking (and any power mocking) I do, the more I wonder how much I am really testing the code and not my ability to string together the right mock call and response sequences, though if it is the only way to get it done, it is still better than nothing.
- Nick Reich
On July 26, 2017, 5:37 p.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61143/
> -----------------------------------------------------------
>
> (Updated July 26, 2017, 5:37 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
>
>
> Bugs: GEODE-3310
> https://issues.apache.org/jira/browse/GEODE-3310
>
>
> Repository: geode
>
>
> Description
> -------
>
> Set target node in TXStateProxy during TXFailover if necessary
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommand.java 6bd00c0
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/MyTransactionFunction.java 0970836
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java 7a56644
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommandTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/61143/diff/1/
>
>
> Testing
> -------
>
> Ongoing precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 61143: GEODE-3310 Set target node in TXStateProxy
during TXFailover if necessary
Posted by Nick Reich <nr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61143/#review181588
-----------------------------------------------------------
Ship it!
Ship It!
- Nick Reich
On July 27, 2017, 5:32 p.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61143/
> -----------------------------------------------------------
>
> (Updated July 27, 2017, 5:32 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
>
>
> Bugs: GEODE-3310
> https://issues.apache.org/jira/browse/GEODE-3310
>
>
> Repository: geode
>
>
> Description
> -------
>
> Set target node in TXStateProxy during TXFailover if necessary
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommand.java 6bd00c0
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/MyTransactionFunction.java 0970836
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java 7a56644
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommandTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/61143/diff/2/
>
>
> Testing
> -------
>
> Ongoing precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 61143: GEODE-3310 Set target node in TXStateProxy
during TXFailover if necessary
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61143/#review181585
-----------------------------------------------------------
geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
Lines 184 (patched)
<https://reviews.apache.org/r/61143/#comment257168>
The logging is to try to help debugging if it ever failed. Most of the time, those unit test failure are hard to reproduce. It will be better to have enough artifacts saved.
You raise a good point of the type of TransactionException. Currently we throw two types of TransactionException if transaction lands on two different nodes -- TransactionDataNodeHasDepartedException and TransactionDataNotColocatedException. However, product does not have enough information to detect in this case if user targets wrong bucket/node (TransactionDataNotColocatedException) or the bucket primary has been moved due to rebalance of node failure. Therefore, the non colocated message is important in this case.
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommandTest.java
Lines 64 (patched)
<https://reviews.apache.org/r/61143/#comment257171>
It will need to have client-server dunit test to test TXFailoverCommand. And it will be hard to trace/track which node to bring down (only has TXStatePeerStub). Also, you can not control which node to failover to in the test -- you need to failover to another server which does not host the TXState.
- Eric Shu
On July 27, 2017, 5:32 p.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61143/
> -----------------------------------------------------------
>
> (Updated July 27, 2017, 5:32 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
>
>
> Bugs: GEODE-3310
> https://issues.apache.org/jira/browse/GEODE-3310
>
>
> Repository: geode
>
>
> Description
> -------
>
> Set target node in TXStateProxy during TXFailover if necessary
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommand.java 6bd00c0
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/MyTransactionFunction.java 0970836
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java 7a56644
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommandTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/61143/diff/2/
>
>
> Testing
> -------
>
> Ongoing precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>
Re: Review Request 61143: GEODE-3310 Set target node in TXStateProxy
during TXFailover if necessary
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61143/
-----------------------------------------------------------
(Updated July 27, 2017, 5:32 p.m.)
Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
Changes
-------
Fix review comments.
Bugs: GEODE-3310
https://issues.apache.org/jira/browse/GEODE-3310
Repository: geode
Description
-------
Set target node in TXStateProxy during TXFailover if necessary
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommand.java 6bd00c0
geode-core/src/test/java/org/apache/geode/internal/cache/execute/MyTransactionFunction.java 0970836
geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java 7a56644
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommandTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/61143/diff/2/
Changes: https://reviews.apache.org/r/61143/diff/1-2/
Testing
-------
Ongoing precheckin.
Thanks,
Eric Shu
Re: Review Request 61143: GEODE-3310 Set target node in TXStateProxy
during TXFailover if necessary
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61143/#review181515
-----------------------------------------------------------
Ship it!
Ship It!
- Darrel Schneider
On July 26, 2017, 10:37 a.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61143/
> -----------------------------------------------------------
>
> (Updated July 26, 2017, 10:37 a.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Lynn Gallinat, and Nick Reich.
>
>
> Bugs: GEODE-3310
> https://issues.apache.org/jira/browse/GEODE-3310
>
>
> Repository: geode
>
>
> Description
> -------
>
> Set target node in TXStateProxy during TXFailover if necessary
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommand.java 6bd00c0
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/MyTransactionFunction.java 0970836
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java 7a56644
> geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/TXFailoverCommandTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/61143/diff/1/
>
>
> Testing
> -------
>
> Ongoing precheckin.
>
>
> Thanks,
>
> Eric Shu
>
>