-
Notifications
You must be signed in to change notification settings - Fork 31
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
Document how to access trailing_metadata from ActiveCall::Operation on streaming responses #241
Comments
In your PR you have the following test: def test_chat_with_metadata
options = Gapic::CallOptions.new metadata: {
'showcase-trailer': ["so", "much", "chat"],
quiet: ["please"]
}
stream_input = Gapic::StreamInput.new
@client.chat stream_input, options do |response_enum, operation|
chatty_thread = Thread.new do
sleep rand
["a", "b", "cee"].each { |x| stream_input.push content: x }
stream_input.close
assert_equal ["a", "b", "cee"], response_enum.to_a.map(&:content)
end
chatty_thread.join
assert_equal(
{ 'showcase-trailer' => ["so", "much", "chat"] },
operation.trailing_metadata
)
end
end Another way or writing that is to use two threads, the first for sending the inputs, and the second for calling def test_chat_with_metadata
options = Gapic::CallOptions.new metadata: {
'showcase-trailer': ["so", "much", "chat"],
quiet: ["please"]
}
stream_input = Gapic::StreamInput.new
@client.chat stream_input, options do |response_enum, operation|
Thread.new do
sleep rand
["a", "b", "cee"].each { |x| stream_input.push content: x }
stream_input.close
assert_equal ["a", "b", "cee"], response_enum.to_a.map(&:content)
end
Thread.new do
operation.wait
assert_equal(
{ 'showcase-trailer' => ["so", "much", "chat"] },
operation.trailing_metadata
)
end
end
end |
I'm not sure if I understand that comment. The issue here is if we want to decorate, wrap, or somehow change the What's the significance of one examples vs the other above? |
The significance is you can swap the call to |
Ok, sure, documenting this as the pattern is one solution. Having a different API, such as a metadata callback, is another solution though and that's what I wanted to open this issue for. I don't have a strong preference for what we use in the tests now given the current state so if you do just put a comment in the PR and I'll update it, add a second test, etc. |
I've explored ways to provide a thread for iterating the streaming response, but there are always unintended consequences for doing so. I believe the best solution is to pass the streaming response (Enumerable) to the user and let the user manage the concurrency for pulling responses out of it. I also think We can document this pattern in the generated docs so the users are aware of how to perform operations like this. |
Currently the underlying
GRPC::ActiveCall::Operation
is exposed to users directly for every method call and it is the only way to access some gRPC properties, like thetrailing_metadata
.This could be confusing because it is low-level and access to the these properties (i.e metadata) depends on the timing of the call. For example,
trailing_metadata
is not available immediately for a streaming call but it is for unary calls (see #239).Do we need a higher level API for this?
The text was updated successfully, but these errors were encountered: