-
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
Use bidirectional streaming RPCs in DistributedTransactionService #228
Use bidirectional streaming RPCs in DistributedTransactionService #228
Conversation
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! It's way more clean than I assumed! Great work 🎉
It looks pretty good but I want to check it again to understand it better.
Left one clarifying question.
return new GrpcTransaction( | ||
response.getTransactionId(), stub, metadataManager, namespace, tableName); | ||
}); | ||
GrpcTransactionService service = new GrpcTransactionService(stub, metadataManager); |
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.
I'm wondering if it should create GrpcTransactionService
object in every start.
If GrpcTransactionManager
object singleton is shared (, which is most of the cases,) it seems not really effective?
Or, should GrpcTransactionManager
be created for each transaction?
Ah, probably, it's created for managing finished
and the latch separately?
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!
Ah, probably, it's created for managing finished and the latch separately?
Yes, GrpcTransactionService
indicates things like a transaction session (assigned to a gRPC bidirectional stream), so we need to instantiate it per transaction. Maybe, the name (GrpcTransactionService) is not good. I will try to rename it to a better one. Thanks!
fd7472e
to
5839fae
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.
LGTM! Thank you!
Left a minor naming suggestion.
import javax.annotation.concurrent.NotThreadSafe; | ||
|
||
@NotThreadSafe | ||
public class GrpcTransactionBidirectionalStream implements StreamObserver<TransactionResponse> { |
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.
public class GrpcTransactionBidirectionalStream implements StreamObserver<TransactionResponse> { | |
public class GrpcTransactionOnBidirectionalStream implements StreamObserver<TransactionResponse> { |
maybe?
5839fae
to
2af57be
Compare
This PR changes
DistributedTransactionService
to use bidirectional streaming RPCs in gRPC.After it's merged, I will work on changing
DistributedStorageService
to use bidirectional streaming RPCs for scan operations, as well.Please take a look!