----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47543/#review133799 -----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RemoteOperationMessage.java (line 242) <https://reviews.apache.org/r/47543/#comment198408> I think this code would be easier to understand if you refactored the if expression, comment, and log statement into a new method. You could call it "hasTxAlreadyFinished(tx)". Your comment can become the method comment (i.e. returns true when handling a late arrival ...) and it would be good to only do the "new TXId" call once (instead of twice). If tx is null you could have the method immediately return false. Otherwise create the TXId, call isHostedTxRecentlyCompleted on it, do the log. Then this caller code would just be: if (!hasTxAlreadyFinished(tx)) { sendReply = operateOnRegion(dm, r, startTime); } geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RemoteOperationMessage.java (line 244) <https://reviews.apache.org/r/47543/#comment198407> It is not clear if this comment applies to the info log msg or the else. Put the comment at the start of the block it applies to. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RemoteOperationMessage.java (line 249) <https://reviews.apache.org/r/47543/#comment198406> don't use this style (single statement with else). Instead put the statement in curly braces and on its own line. geode-core/src/main/java/com/gemstone/gemfire/internal/cache/TXManagerImpl.java (line 746) <https://reviews.apache.org/r/47543/#comment198417> could unit tests be added for these new TXManagerImpl methods? geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/PartitionMessage.java (line 323) <https://reviews.apache.org/r/47543/#comment198409> same comments as RemoteOperationMessage - Darrel Schneider On May 18, 2016, 10:21 a.m., Eric Shu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47543/ > ----------------------------------------------------------- > > (Updated May 18, 2016, 10:21 a.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 > > Diff: https://reviews.apache.org/r/47543/diff/ > > > Testing > ------- > > precheckin > > > Thanks, > > Eric Shu > >
