----------------------------------------------------------- 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 > >
