Skip to content
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

fix(common): use original options when resuming streaming RPCs #8404

Conversation

coryan
Copy link
Contributor

@coryan coryan commented Feb 17, 2022

From the discussion on #8355 (comment)


This change is Reviewable

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 6d43ca30ad106be9ec34820b04d7b120b2a028ae

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@coryan coryan changed the title fix(common): use original options when resuming streaming rpcs fix(common): use original options when resuming streaming RPCs Feb 17, 2022
@coryan coryan marked this pull request as ready for review February 17, 2022 13:37
@coryan coryan requested a review from a team as a code owner February 17, 2022 13:37
@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #8404 (f74c2af) into main (d6e0897) will decrease coverage by 0.00%.
The diff coverage is 84.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8404      +/-   ##
==========================================
- Coverage   95.04%   95.03%   -0.01%     
==========================================
  Files        1363     1363              
  Lines      121304   121328      +24     
==========================================
+ Hits       115290   115306      +16     
- Misses       6014     6022       +8     
Impacted Files Coverage Δ
...ogle/cloud/internal/resumable_streaming_read_rpc.h 85.71% <33.33%> (-6.18%) ⬇️
...loud/internal/resumable_streaming_read_rpc_test.cc 91.25% <100.00%> (+1.01%) ⬆️
...e/cloud/spanner/testing/cleanup_stale_instances.cc 48.38% <0.00%> (-12.91%) ⬇️
...ud/spanner/integration_tests/client_stress_test.cc 85.52% <0.00%> (-0.66%) ⬇️
.../cloud/storage/benchmarks/throughput_experiment.cc 74.37% <0.00%> (-0.51%) ⬇️
google/cloud/pubsub/samples/samples.cc 92.02% <0.00%> (-0.24%) ⬇️
...cloud/pubsub/internal/subscription_session_test.cc 98.00% <0.00%> (+0.24%) ⬆️
google/cloud/bigtable/async_read_stream_test.cc 97.99% <0.00%> (+0.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6e0897...f74c2af. Read the comment docs.

@coryan coryan force-pushed the fix-common-option-span-for-streaming-read-rpcs branch from 6d43ca3 to f74c2af Compare February 18, 2022 01:43
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: f74c2afed21cf81e043cb2d79ad64f802f925d33

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

Copy link
Contributor

@devbww devbww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about google::cloud::internal::ResumableStreamingReadRpc<>, I just created #8404, you can tell me there if the changes are not needed.

No, I don't think these changes are needed. That is, these new OptionsSpans are redundant, and we probably should avoid creating them, if only for performance reasons. It is also nice to have the span stuff all in one place, and as close to the user as possible.

In this case, a ResumableStreamingReadRpc is always immediately transferred into a StreamReader<T> functor, and that is then moved into a StreamRange<T> via MakeStreamRange<T>(). From there, the reader is only ever invoked/destroyed under the restored options, as the user handles the StreamRange<T>, due to the changes already made to google/cloud/stream_range.h.

@coryan coryan closed this Feb 18, 2022
@coryan coryan deleted the fix-common-option-span-for-streaming-read-rpcs branch February 18, 2022 12:54
@coryan
Copy link
Contributor Author

coryan commented Feb 18, 2022

Thanks for looking into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants