GEODE-5828: fixes replicates miss transaction commit. #2571
Conversation
* Always send CommitProcessQueryMessage to other replicates to see if anyone received second message as transaction originator for client transaction will be different from transation host. * Do not wait for transaction originator to depart when transaction host is crashed. Originator may not be the same as the crashed transaction host and never departs from distributed system.
| @@ -1979,7 +1978,7 @@ public void memberDeparted(DistributionManager distributionManager, | |||
| if (!getSender().equals(id)) { | |||
| return; | |||
| } | |||
| this.dm.removeMembershipListener(this); | |||
| getDm().removeMembershipListener(this); | |||
There was a problem hiding this comment.
instead of "getDm()" use the parameter "distributionManager"
| return farSiders; | ||
| } | ||
|
|
||
| DistributionManager getDm() { |
There was a problem hiding this comment.
rename "getDm" to "getDistributionManager"
| } | ||
|
|
||
| void doCommitProcessQuery(final InternalDistributedMember id) { | ||
| final TXCommitMessage message = TXCommitMessage.this; |
There was a problem hiding this comment.
What class is this method "doCommitProcessQuery" on? I think it is TXCommitMessage.
So no need for this funny business of "TXCommitMessage.this" which was needed before because is was in an anonymous inner class.
Now everywhere you have "message" you can just have "this".
I already did this refactoring in the pull request that removes thread groups.
I would suggest for your fix not to refactor this chunk of code into its own method. Just delete the block of code from it that you want. That way it will be more clear what you are fixing and you will not have conflicts with the thread refactoring code.
There was a problem hiding this comment.
You are right that it is no longer in inner class. Cleaned up the code.
| } | ||
|
|
||
| boolean foundTxInProgress(TXCommitMessage message) { | ||
| if (null != message && message.isProcessing()) { |
There was a problem hiding this comment.
simplify to: return null != message && message.isProcessing();
pivotal-eshu
requested review from
dschneider-pivotal,
kirklund,
agingade and
lgallinat
| @@ -1979,7 +1978,7 @@ public void memberDeparted(DistributionManager distributionManager, | |||
| if (!getSender().equals(id)) { | |||
| return; | |||
| } | |||
| this.dm.removeMembershipListener(this); | |||
| getDistributionManager().removeMembershipListener(this); | |||
There was a problem hiding this comment.
it would be better to just use the "distributionManager" parameter. At some point we may find that this class no longer needs to keep a reference to the dm in an instance field.
There was a problem hiding this comment.
Will address this concern with additional dunit test.
| @@ -1837,7 +1836,7 @@ public CommitProcessQueryMessage(Object trackerKey, int processorId) { | |||
|
|
|||
| @Override | |||
| protected void process(ClusterDistributionManager dm) { | |||
| final boolean processMsgReceived = txTracker.commitProcessReceived(this.trackerKey, dm); | |||
| final boolean processMsgReceived = txTracker.commitProcessReceived(this.trackerKey); | |||
There was a problem hiding this comment.
I think the old code was better that passed along the "dm" parameter.
There was a problem hiding this comment.
txTracker.commitProcessReceived method no longer need distribution manager anymore.
* Always send CommitProcessQueryMessage to other replicates to see if anyone received second message as transaction originator for client transaction will be different from transation host. * Do not wait for transaction originator to depart when transaction host is crashed. Originator may not be the same as the crashed transaction host and never departs from distributed system. (cherry picked from commit db8ba67)
message as transaction originator for client transaction will be different from transaction host.
transaction host is crashed. Originator may not be the same as the crashed transaction host and
never departs from the distributed system.