----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47543/#review134651 -----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RemoteOperationMessage.java (line 208) <https://reviews.apache.org/r/47543/#comment199497> A new rule of thumb: If an "if" block ends with return then you should get rid of the "else" block. This helps prevent a bunch of extra identation. So I'd change this method to: if (tx == null) { return false; } TXId txid = new TXId(getMemberToMasqueradeAs(), getTXUniqId()); if (hasTXRecentlyCompleted(txid, txMgr)) { ... return true; } return false; geode-core/src/main/java/com/gemstone/gemfire/internal/cache/TXManagerImpl.java (line 746) <https://reviews.apache.org/r/47543/#comment199498> I think you just wanted to call this from unit tests so why make it public? Seems like the default access level (like you did on getLock) would have been enough. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/PartitionMessage.java (line 281) <https://reviews.apache.org/r/47543/#comment199499> I noticed this method and hasTXRecentlyCompleted are duplicated. They are in both PartitionMessage and RemoteOperationMessage. Seems like you could have them in just one place. You could hoist them up to the parent class (DistributionMessage) but it seems like it might be better to add a method to TXManagerImpl that has this logic. geode-core/src/test/java/com/gemstone/gemfire/internal/cache/RemoteOperationMessageTest.java (line 71) <https://reviews.apache.org/r/47543/#comment199496> I see a bunch of places in your unit tests that are catching an exception and either ignoring it or calling printStackTrace and then ignoring it. I think you should remove all of these try/catch blocks and instead just have the method throw that exception. You might need to add "throws ..." on the methods including the top level test but that is the best way in unit tests. You should only have catch if the test is expecting the exception and will fail if it does not see it (and you can usually do this with some type of ExpectedException handler instead of try/catch). - Darrel Schneider On May 24, 2016, 3:05 p.m., Eric Shu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47543/ > ----------------------------------------------------------- > > (Updated May 24, 2016, 3:05 p.m.) > > > Review request for geode, Darrel Schneider and Swapnil Bawaskar. > > > Repository: geode > > > Description > ------- > > This also handles when an inflight msg arrived after the transaction is > completed after a failover. > > > Diffs > ----- > > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RemoteOperationMessage.java > 42ce811 > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/TXManagerImpl.java > 4b2f904 > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/PartitionMessage.java > 626efef > > geode-core/src/test/java/com/gemstone/gemfire/internal/cache/RemoteOperationMessageTest.java > PRE-CREATION > > geode-core/src/test/java/com/gemstone/gemfire/internal/cache/TXManagerImplTest.java > PRE-CREATION > > geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PartitionMessageTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/47543/diff/ > > > Testing > ------- > > precheckin > > > Thanks, > > Eric Shu > >
