-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor the integration tests for transaction #215
Refactor the integration tests for transaction #215
Conversation
6770e47
to
29bd058
Compare
29bd058
to
0d42999
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
The code looks good but I don't know why we use ConsensusCommit
and JdbcTransaction
instead of DistributedTransaction
.
@@ -841,14 +837,14 @@ public void get_GetGivenForDeletedWhenCoordinatorStateNotExistAndExpired_ShouldA | |||
public void getAndScan_CommitHappenedInBetween_ShouldReadRepeatably() | |||
throws CrudException, CommitException, UnknownTransactionStatusException { | |||
// Arrange | |||
DistributedTransaction transaction = manager.start(); | |||
ConsensusCommit transaction = manager.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question. Is there any reason of using ConsensusCommit
instead of DistributedTransaction
?
(usually we should use DistributedTransaction
, right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for reviewing this!
The reason for those changes is that the integration tests are actually just for ConsensusCommit
, but not for general DistributedTransaction
(I found that when I tried to use it for gRPC transactions, but I couldn't). To highlight that, I made those changes, but they are not very important. And yes, usually we should use DistributedTransaction
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! (from mobile)
This is a very minor change. Please take a look!