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.

Sign up for GitHub

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

Jump to bottom

impl(pubsub): add more than 128 links #12928

Merged
merged 22 commits into from
Oct 25, 2023
Merged

impl(pubsub): add more than 128 links #12928

merged 22 commits into from
Oct 25, 2023

Conversation

alevenberg
Copy link
Member

@alevenberg alevenberg commented Oct 19, 2023 •
edited
Loading

Part of #12528

This change is Reviewable

@alevenberg alevenberg requested a review from a team as a code owner October 19, 2023 17:47
@alevenberg alevenberg temporarily deployed to internal October 19, 2023 17:47 — with GitHub Actions Inactive
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the Pub/Sub API. label Oct 19, 2023
@codecov
Copy link

codecov bot commented Oct 19, 2023 •
edited
Loading

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f931c14) 93.62% compared to head (1fde8d7) 93.62%.
Report is 32 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #12928   +/-   ##
=======================================
  Coverage   93.62%   93.62%           
=======================================
  Files        2067     2067           
  Lines      180210   180228   +18     
=======================================
+ Hits       168715   168733   +18     
  Misses      11495    11495
Files Coverage Δ
...d/pubsub/internal/batching_publisher_connection.cc 100.00% <100.00%> (ø)
...sub/internal/batching_publisher_connection_test.cc 99.63% <100.00%> (+<0.01%) ⬆️
google/cloud/pubsub/internal/message_batch.h 100.00% <ø> (ø)
google/cloud/pubsub/internal/noop_message_batch.h 100.00% <100.00%> (ø)
...ogle/cloud/pubsub/internal/tracing_message_batch.h 100.00% <ø> (ø)
google/cloud/pubsub/testing/mock_message_batch.h 100.00% <100.00%> (ø)
google/cloud/testing_util/opentelemetry_matchers.h 100.00% <100.00%> (ø)
...gle/cloud/pubsub/internal/tracing_message_batch.cc 98.57% <97.95%> (+0.42%) ⬆️
...loud/pubsub/internal/tracing_message_batch_test.cc 99.39% <98.11%> (-0.61%) ⬇️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

coryan
coryan reviewed Oct 19, 2023
View reviewed changes
Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

Quick drive by

google/cloud/pubsub/internal/tracing_message_batch.cc Outdated Show resolved Hide resolved
google/cloud/pubsub/internal/tracing_message_batch.cc Outdated Show resolved Hide resolved
google/cloud/pubsub/internal/tracing_message_batch_test.cc Outdated Show resolved Hide resolved
google/cloud/pubsub/internal/tracing_message_batch_test.cc Outdated Show resolved Hide resolved
@alevenberg alevenberg temporarily deployed to internal October 19, 2023 19:07 — with GitHub Actions Inactive
@alevenberg alevenberg temporarily deployed to internal October 19, 2023 19:13 — with GitHub Actions Inactive
@alevenberg alevenberg temporarily deployed to internal October 19, 2023 22:29 — with GitHub Actions Inactive
coryan
coryan approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

I don't want to block progress, but it feels that this could use another pass. If you need to commit this to make progress on other work, please go ahead. If it can wait until Monday let's do that.

google/cloud/pubsub/internal/tracing_message_batch.cc Outdated Show resolved Hide resolved
google/cloud/pubsub/internal/tracing_message_batch_test.cc Outdated Show resolved Hide resolved
google/cloud/pubsub/internal/tracing_message_batch.cc Outdated Show resolved Hide resolved
google/cloud/pubsub/internal/tracing_message_batch.cc Outdated Show resolved Hide resolved
@alevenberg alevenberg added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 20, 2023
@alevenberg
Copy link
Member Author

I started refactoring, but looks like it’s gonna have to wait to Monday.

@alevenberg alevenberg temporarily deployed to internal October 22, 2023 22:29 — with GitHub Actions Inactive
@alevenberg alevenberg temporarily deployed to internal October 22, 2023 22:34 — with GitHub Actions Inactive
@alevenberg alevenberg removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 22, 2023
@alevenberg alevenberg requested review from devbww and coryan October 22, 2023 23:00
@alevenberg alevenberg temporarily deployed to internal October 23, 2023 00:08 — with GitHub Actions Inactive
devbww
devbww reviewed Oct 23, 2023
View reviewed changes
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.

Just some surface-level questions/comments.

google/cloud/pubsub/internal/tracing_message_batch.cc Show resolved Hide resolved
google/cloud/pubsub/internal/tracing_message_batch.cc Show resolved Hide resolved
google/cloud/pubsub/internal/tracing_message_batch.cc Outdated Show resolved Hide resolved
@alevenberg alevenberg temporarily deployed to internal October 24, 2023 20:38 — with GitHub Actions Inactive
@alevenberg alevenberg temporarily deployed to internal October 25, 2023 13:28 — with GitHub Actions Inactive
@alevenberg alevenberg temporarily deployed to internal October 25, 2023 13:31 — with GitHub Actions Inactive
coryan
coryan approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

A few nits..

google/cloud/pubsub/internal/batching_publisher_connection.cc Outdated
batch.batch = batch_;
sink_->AsyncPublish(std::move(request)).then(std::move(batch));
auto handler = batch_->Flush();
sink_->AsyncPublish(std::move(request)).then(std::move(batch)).then(std::move(handler));
Copy link
Contributor

Choose a reason for hiding this comment

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

This line makes all your suffering through this long review worth it 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is much cleaner than how it started. I hope it makes your suffering through the review worth it as well 🥳

google/cloud/pubsub/internal/tracing_message_batch.cc Outdated Show resolved Hide resolved
google/cloud/pubsub/internal/tracing_message_batch.h Show resolved Hide resolved
google/cloud/pubsub/internal/tracing_message_batch_test.cc Outdated Show resolved Hide resolved
google/cloud/pubsub/testing/mock_message_batch.h Outdated Show resolved Hide resolved
google/cloud/pubsub/internal/message_batch.h Outdated Show resolved Hide resolved
@alevenberg alevenberg temporarily deployed to internal October 25, 2023 13:49 — with GitHub Actions Inactive
@alevenberg alevenberg enabled auto-merge (squash) October 25, 2023 13:50
@alevenberg alevenberg temporarily deployed to internal October 25, 2023 14:03 — with GitHub Actions Inactive
@alevenberg alevenberg temporarily deployed to internal October 25, 2023 15:58 — with GitHub Actions Inactive
@alevenberg alevenberg temporarily deployed to internal October 25, 2023 16:57 — with GitHub Actions Inactive
@alevenberg alevenberg merged commit ec38e4b into googleapis:main Oct 25, 2023
@alevenberg alevenberg deleted the 1-2 branch October 25, 2023 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devbww devbww devbww left review comments

@coryan coryan coryan approved these changes

Assignees
No one assigned
Labels
api: pubsub Issues related to the Pub/Sub API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alevenberg @coryan @devbww