-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implement metadata in __call__ for aio unaryunary call #12
Implement metadata in __call__ for aio unaryunary call #12
Conversation
|
||
class _TestServiceServicer(test_pb2_grpc.TestServiceServicer): | ||
|
||
async def UnaryCall(self, request, context): | ||
# _maybe_echo_metadata(context) |
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 can not get metadata from here until server-side implement it, the context
is a placeholder[1] and server-side just send fake metadata[2] now, so I comment this line.
[1] https://github.com/grpc/grpc/blob/master/src/python/grpcio/grpc/_cython/_cygrpc/aio/server.pyx.pxi#L78
[2] https://github.com/grpc/grpc/blob/master/src/python/grpcio/grpc/_cython/_cygrpc/aio/server.pyx.pxi#L88-L94
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, I left some suggestions, let me know what you think of them.
@@ -139,7 +139,11 @@ cdef class _AioCall: | |||
self._destroy_grpc_call() | |||
|
|||
if receive_status_on_client_operation.code() == StatusCode.ok: | |||
return receive_message_operation.message() | |||
return receive_initial_metadata_operation.initial_metadata(), \ |
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.
Maybe we should create a namedtuple
to make all these values easier to move around, and more readable.
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 will return a namedtuple instead of tuple.
@@ -66,7 +65,8 @@ cdef class _AioCall: | |||
"""Destroys the corresponding Core object for this RPC.""" | |||
grpc_call_unref(self._grpc_call_wrapper.call) | |||
|
|||
async def unary_unary(self, bytes method, bytes request, object timeout, AioCancelStatus cancel_status): | |||
async def unary_unary(self, bytes method, bytes request, object timeout, object metadata, |
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 believe here the type for metadata could be improved
async def unary_unary(self, bytes method, bytes request, object timeout, object metadata, | |
async def unary_unary(self, bytes method, bytes request, object timeout, tuple metadata, |
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.
Because the async api doc[1] define method initial_metadata
of Call
returns a Sequence
, I'm not sure if it's ok to set the tuple type.
I have opened it against grpc repo. |
Implement
metadata
forAio UnaryUnary Call
.Fix grpc#20144