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(pubsub): mark ignore option default for publish flow control #5983

Merged
merged 2 commits into from May 4, 2022

Conversation

hongalex
Copy link
Member

@hongalex hongalex commented May 3, 2022

Make ignore flow control the default option for publisher flow control. In addition, fix bug where default settings were not being respected.

Fixes #5976

@hongalex hongalex requested review from a team as code owners May 3, 2022 20:07
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: pubsub Issues related to the Pub/Sub API. labels May 3, 2022
Copy link
Contributor

@shollyman shollyman left a comment

Choose a reason for hiding this comment

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

Re-ordering the values via the iota is suspicious to me. If users are using the names consistently it is reasonable, but if they've been setting the int values directly or define their own enum-like things this will potentially cause a behavior change.

@hongalex
Copy link
Member Author

hongalex commented May 3, 2022

Re-ordering the values via the iota is suspicious to me. If users are using the names consistently it is reasonable, but if they've been setting the int values directly or define their own enum-like things this will potentially cause a behavior change.

Yeah I didn't really want to do this but due to a bug/happenstance this is not a breaking change. There was a bug in the code where we compare the t.PublishSettings.FlowControlSettings.LimitExceededBehavior field (default Block since it's the zero value) to LimitExceededBlock. However, since the default value is actually Ignore, users have been unable to set this to Block.

With this change, Ignore should be the default option both in the DefaultReceiveSettings struct as well with the enum and users will be able to use both Ignore and Block properly.

edit: Also, the iota reordering has to happen. As is, we cannot differentiate between if the user didn't set the LimitExceededBehavior field or if they intentionally want to use Block. If the user doesn't change this field, we need to use the default value (which is Ignore).

Copy link
Contributor

@shollyman shollyman left a comment

Choose a reason for hiding this comment

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

LGTM given the additional context.

Copy link

@feywind feywind left a comment

Choose a reason for hiding this comment

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

Approving on behalf of others not in the groups.

@hongalex hongalex added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 4, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 4, 2022
@hongalex hongalex merged commit 3f41531 into googleapis:main May 4, 2022
@hongalex hongalex deleted the fix-pubsub-fc branch May 4, 2022 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pubsub: incorrect default value for PublishSettings LimitExceededBehavior
4 participants