You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Eric Shu <es...@pivotal.io> on 2016/05/18 17:21:30 UTC
Review Request 47543: GEODE-1400, Handle inflight p2p msg late arrival
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47543/
-----------------------------------------------------------
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
Re: Review Request 47543: GEODE-1400,
Handle inflight p2p msg late arrival
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 47543: GEODE-1400,
Handle inflight p2p msg late arrival
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 47543: GEODE-1400,
Handle inflight p2p msg late arrival
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47543/#review135022
-----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RemoteOperationMessage.java (line 238)
<https://reviews.apache.org/r/47543/#comment200009>
One of the things I see is that PartitionMessage and RemoteOperationMessage have a bunch of transaction related code in common.
I think this is because they both implement the TransactionMessage interface.
I think we should introduce a new abstract class AbstractTransactionMessage that this shared code can live in. It would extend DistributionMessage.
Then these two classes could extend it instead of DistributionMessage. It is also possible that other messages that implement the TransactionMessage interface could instead extend this AbstractTransactionMessage.
So instead of doing this work as part of this ticket I'd just file a jira ticket saying that these classes have code duplication and should be refactored.
- 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
>
>
Re: Review Request 47543: GEODE-1400,
Handle inflight p2p msg late arrival
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47543/#review135252
-----------------------------------------------------------
Ship it!
Ship It!
- Darrel Schneider
On May 26, 2016, 10:31 p.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47543/
> -----------------------------------------------------------
>
> (Updated May 26, 2016, 10:31 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
>
>
Re: Review Request 47543: GEODE-1400,
Handle inflight p2p msg late arrival
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47543/
-----------------------------------------------------------
(Updated May 27, 2016, 5:31 a.m.)
Review request for geode, Darrel Schneider and Swapnil Bawaskar.
Changes
-------
Remove unused variables in the test. Adding pollInterval and pollDelay.
Repository: geode
Description
-------
This also handles when an inflight msg arrived after the transaction is completed after a failover.
Diffs (updated)
-----
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
Re: Review Request 47543: GEODE-1400,
Handle inflight p2p msg late arrival
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47543/#review135109
-----------------------------------------------------------
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/TXManagerImplTest.java (line 50)
<https://reviews.apache.org/r/47543/#comment200131>
I think you no longer use count so you should delete it
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/TXManagerImplTest.java (line 195)
<https://reviews.apache.org/r/47543/#comment200135>
I would add ".pollInterval(10, TimeUnit.MILLISECONDS).pollDelay(10, TimeUnit.MILLISECONDS)".
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/TXManagerImplTest.java (line 209)
<https://reviews.apache.org/r/47543/#comment200133>
Shouldn't you "assertTrue(latch.await(15...))"?
Also I'd increase this to 60 seconds.
- Darrel Schneider
On May 26, 2016, 3:44 p.m., Eric Shu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47543/
> -----------------------------------------------------------
>
> (Updated May 26, 2016, 3:44 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
>
>
Re: Review Request 47543: GEODE-1400,
Handle inflight p2p msg late arrival
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47543/
-----------------------------------------------------------
(Updated May 26, 2016, 10:44 p.m.)
Review request for geode, Darrel Schneider and Swapnil Bawaskar.
Changes
-------
Make TXManagerImpl mockable so it can be mocked in the unit test.
Only unmasqerade when TXStateProxy is masqeraded.
Avoid to use Thread.sleep() in the unit test.
Repository: geode
Description
-------
This also handles when an inflight msg arrived after the transaction is completed after a failover.
Diffs (updated)
-----
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
Re: Review Request 47543: GEODE-1400,
Handle inflight p2p msg late arrival
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 47543: GEODE-1400,
Handle inflight p2p msg late arrival
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47543/
-----------------------------------------------------------
(Updated May 25, 2016, 11:58 p.m.)
Review request for geode, Darrel Schneider and Swapnil Bawaskar.
Changes
-------
Move the hasTxAlreadyFinished method to TXManagerImpl. Modify unit test accrodingly.
Repository: geode
Description
-------
This also handles when an inflight msg arrived after the transaction is completed after a failover.
Diffs (updated)
-----
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
Re: Review Request 47543: GEODE-1400,
Handle inflight p2p msg late arrival
Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47543/
-----------------------------------------------------------
(Updated May 24, 2016, 10:05 p.m.)
Review request for geode, Darrel Schneider and Swapnil Bawaskar.
Changes
-------
Fixed issues and added unit tests.
Repository: geode
Description
-------
This also handles when an inflight msg arrived after the transaction is completed after a failover.
Diffs (updated)
-----
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