----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47543/#review135008 -----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RemoteOperationMessage.java (line 241) <https://reviews.apache.org/r/47543/#comment199995> the masqueradeAs should be right before the try instead of right after. So instead of "TXStateProxy tx = null;" you would have "TXStateProxy tx = masqueradeAs(this, txMgr); geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RemoteOperationMessage.java (line 242) <https://reviews.apache.org/r/47543/#comment200002> Something I don't like about this new code is that we now call "new TXId(...)" even when we are not doing a transaction. Perhaps you could add the try/finally to a method named "operateOnTx(tx, txMgr, dm, r, startTime)". Then this calling code would look like this: TXStateProxy tx = masqueradeAs(...); if (tx != null) { sendReply = operateOnTx(tx, txMgr, dm, r, startTime); } else { sendReply = operateOnRegion(dm, r, startTime); } geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RemoteOperationMessage.java (line 324) <https://reviews.apache.org/r/47543/#comment199996> It seems really odd to have an instance method that takes "this" as a parameter. I think you introduced this method for mocking but wonder if it can be done in a better way. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/PartitionMessage.java (line 346) <https://reviews.apache.org/r/47543/#comment200004> See comments on RemoteOperationMessage geode-core/src/test/java/com/gemstone/gemfire/internal/cache/TXManagerImplTest.java (line 189) <https://reviews.apache.org/r/47543/#comment200007> sleeps in unit tests will probably introduce a flaky test. Could you coordinate these two threads with locks or some other concurrent object? - Darrel Schneider On May 25, 2016, 4:58 p.m., Eric Shu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47543/ > ----------------------------------------------------------- > > (Updated May 25, 2016, 4:58 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 > >
