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

fix: update grpc.max_metadata_size to 4MiB for exactly-once, and shift ack/modack errors to 'debug' stream channel #1505

Merged
merged 8 commits into from
Apr 6, 2022
Merged

fix: update grpc.max_metadata_size to 4MiB for exactly-once, and shift ack/modack errors to 'debug' stream channel #1505

merged 8 commits into from
Apr 6, 2022

Conversation

feywind
Copy link
Collaborator

@feywind feywind commented Mar 21, 2022 •
edited
Loading

As the title says, this sets a default value for grpc.max_metadata_size.

While fixing that, I discovered that we are incorrectly raising a client error for ack/modack failures. Rather than making these go silent for users (for debug purposes), I moved them to a 'debug' channel that can also be subscribed on Subscription instances.

Fixes #1504 🦕

@feywind feywind added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 21, 2022
@feywind feywind requested review from a team as code owners March 21, 2022 22:06
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the googleapis/nodejs-pubsub API. label Mar 21, 2022
pradn
pradn reviewed Mar 21, 2022
View reviewed changes
Copy link

@pradn pradn left a comment

Choose a reason for hiding this comment

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

Looks good. Please test:

  1. subscribing for a long time from a exactly-once delivery-enabled subscription
  2. subscribing from a regular subscription and turning on exactly-once for the subscription

In both cases, the subscribing should continue as normal.

@feywind feywind added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 24, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 24, 2022
@feywind feywind added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 1, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 1, 2022
@feywind feywind removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 1, 2022
@feywind feywind changed the title fix: update grpc.max_metadata_size to 4MiB for exactly-once fix: update grpc.max_metadata_size to 4MiB for exactly-once, and shift ack/modack errors to 'debug' stream channel Apr 5, 2022
@feywind
Copy link
Collaborator Author

feywind commented Apr 5, 2022

Looks good. Please test:

  1. subscribing for a long time from a exactly-once delivery-enabled subscription
  2. subscribing from a regular subscription and turning on exactly-once for the subscription

In both cases, the subscribing should continue as normal.

These are tested and pass after the fix for the ack/modack errors.

bcoe
bcoe approved these changes Apr 5, 2022
View reviewed changes
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

Left one nit that might be worth fixing.

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Apr 5, 2022
@feywind feywind added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 6, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 6, 2022
@feywind feywind added the automerge Merge the pull request once unit tests and other checks pass. label Apr 6, 2022
@feywind feywind merged commit abd10cc into googleapis:main Apr 6, 2022
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Apr 6, 2022
@feywind feywind deleted the gh1504-grpc-metadata branch April 6, 2022 19:56
@release-please release-please bot mentioned this pull request Apr 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pradn pradn pradn left review comments

@bcoe bcoe bcoe approved these changes

Assignees
No one assigned
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Increase "grpc.max_metadata_size" GRPC parameter to 4 MB
3 participants
@feywind @bcoe @pradn