-
Notifications
You must be signed in to change notification settings - Fork 214
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
feat: Add Mint adapter #272
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.
The overall work looks great!
lib/grpc/client/adapters/mint/connection_process/connection_process.ex
Outdated
Show resolved
Hide resolved
lib/grpc/client/adapters/mint/connection_process/connection_process.ex
Outdated
Show resolved
Hide resolved
lib/grpc/client/adapters/mint/connection_process/connection_process.ex
Outdated
Show resolved
Hide resolved
… receive data and improve readability
a4a53ea
to
a84d803
Compare
@polvalente I think this one is good to go. There is one this one comment that I've solved, but I like your input about what I saw: #272 (comment) Also, dialyzer is breaking for 24.x because of cache. Since my last successful build til today the latest OTP 24 version was updated from Edit: Found out that you can also delete the cache manually: https://github.com/elixir-grpc/grpc/actions/caches lol |
TIL about manually deleting cache entries! |
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 think we're getting to the final rounds here! I just left some comments improving docs and a last question regarding the stream.
After that's answered I'll do a final pass through the whole PR. I don't think we missed anything, but since this PR went through many iterations, it's best to be safe.
Co-authored-by: Paulo Valente <[email protected]>
Co-authored-by: Paulo Valente <[email protected]>
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.
Awesome work! I left a final batch of comments.
Most are stylistic, but there are some questions as well (which don't seem to block this PR further!)
Once we resolve this last batch we can merge and open an issue to tackle all of the TODOs we left.
lib/grpc/client/adapters/mint/connection_process/connection_process.ex
Outdated
Show resolved
Hide resolved
lib/grpc/client/adapters/mint/connection_process/connection_process.ex
Outdated
Show resolved
Hide resolved
Co-authored-by: Paulo Valente <[email protected]>
…ocess.ex Co-authored-by: Paulo Valente <[email protected]>
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 think we finally got this to a place where it can be merged! I've commited some changes to your latest error messages and we just need to wait for CI :)
Awesome work!
@tony612 I'm merging this so we can have the current state of the adapter as a clean slate. We can discuss next iterations on a future issue/PR if needed. There are already some TODO comments that will be addressed |
Opened an Issue for one of the TODO's: #291 |
@polvalente Got it. Please ping me if there's anything needed from me. Nice to see this got merged! |
Closes: #242
(sorry for the huge PR - tried to make everything well tested + make interop tests work with mint)
Covered in this PR
Cover all possible use cases for a gRPC (client-server):
There is also several E2E test cases here, which includes:
Suggestion on how to review
We have two main modules that handles the interactions with the server.
Connection Process
This process is spawned every time we attempt to connect to a gRPC server. It does strictly three things:
tcp_messages
. And that includesBecause of HTTP2 flow control mechanics I've designed the request handling of all requests to behave as a streamed request, this way it's easier to check for the window size and chunk the request if necessary. Because of that, you might see that I'm treating even unary requests, as streams - but that behavior is transparent to the user of the lib.
I'm also handling connection drops with the following strategy:
Response Process
This process is spawned every time a request is made. It's responsible to receive cast calls from the
ConnectionProcess
and produce the messages to an Elixir.Stream. It dies when the request is finished - so it should be a short lived process.