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

feat: add flow control for message publishing #96

Merged
merged 13 commits into from
Jun 2, 2020
Merged

feat: add flow control for message publishing #96

merged 13 commits into from
Jun 2, 2020

Conversation

plamut
Copy link
Contributor

@plamut plamut commented May 7, 2020 •
edited
Loading

Closes #16.
Closes #72.

This PR adds flow control settings to publisher client, similar to what was added to the Java client recently.

The PR does not interfere with existing publish logic (sequencers, ordering keys, batching...). Instead, it acts like a valve placed in front of the publish pipeline, and taking the configured action if it detects excessive message flow.

Things left to do / discuss

  • A system test for this? Although not sure how feasible would it be, as we would have to publish massive amounts of data on fast connection to have a chance of triggering the flow control error...
  • If the desired behavior is BLOCK, should blocking also support timeouts? However, every publish future gets resolved sooner or later be done (even if due to an error/timeout), and will release the corresponding message from the flow controller then.
  • Unlike Java, the FlowControl settings are placed under PublisherOptions, and not BatchSettings. They fit there much more naturally IMO, but I can change this.
  • What should be the default FlowControl values (the thresholds, specifically)? I set the message count and byte size to 10x the defaults in BatchSettings, would that be fine for most users?
  • Documenting the new feature more extensively than docstring updates in this PR? Can in principle also be done in a separate PR.

PR checklist

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 7, 2020
This was referenced May 7, 2020
Closed
Closed
@plamut plamut added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label May 8, 2020
@plamut
Copy link
Contributor Author

plamut commented May 11, 2020 •
edited
Loading

I added support for multithreaded add(), including the logic to prevent threads adding large messages from starving.

Things left:

  • Improve debug log messages (include info on the current flow controller capacity)
  • ERROR path: raise overflow errors through the publish future, not directly.
  • Add a smoke system test - probably using the ERROR path, as triggering a timeout would be too difficult on reasonably fast connection (we don't want to DOS the infrastructure, after all).
    Added a unit test covering the same, just without hitting the backend (doesn't matter for the flow controller, as this is past it in the pipeline).

@plamut plamut marked this pull request as ready for review May 12, 2020 09:06
@plamut plamut mentioned this pull request May 13, 2020
Closed
@plamut
Copy link
Contributor Author

plamut commented May 28, 2020

@pradn Do you have anything to add here, or are we good to merge?

pradn
pradn requested changes May 28, 2020
View reviewed changes
google/cloud/pubsub_v1/publisher/flow_controller.py Outdated
error_msg = "Flow control limits too low for the message - {}.".format(
load_info
)
raise exceptions.PermanentlyBlockedError(error_msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

"WouldPermanentlyBlockError" makes more sense, because it's not already permanently blocked.

Even better, I think we can simply omit the details about blocking and say something more generic: FlowControlMessageTooLargeError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I will rename the exception.

Copy link
Contributor Author

@plamut plamut May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I eventually opted for the generic FlowControlLimitError for a few reasons:

  • FlowControlMessageTooLargeError could be confusing, as there already exists a similarly named MessageTooLargeError for the messages that would exceed the limits on the backend.
  • A message could block permanently for reasons other than its size, e.g. if flow control's max messages is set to 0 or some other non-sensible value. Mentioning the size would actually be misleading in such case, but a generic FlowControlLimitsError would semantically still be correct.
  • One less exception type to keep track of.

google/cloud/pubsub_v1/publisher/flow_controller.py Show resolved Hide resolved
google/cloud/pubsub_v1/publisher/flow_controller.py Outdated Show resolved Hide resolved
@plamut
Copy link
Contributor Author

plamut commented Jun 2, 2020 •
edited
Loading

@pradn If there's nothing else left, please just give an approval to unblock the merge, thanks! :)

@plamut plamut merged commit 06085c4 into googleapis:master Jun 2, 2020
@plamut plamut deleted the iss-72 branch June 2, 2020 19:09
gcf-merge-on-green bot pushed a commit that referenced this pull request Jun 9, 2020
@release-please
chore: release 1.6.0 (#117)
60fe71a 
## [1.6.0](https://www.github.com/googleapis/python-pubsub/compare/v1.5.0...v1.6.0) (2020-06-09)

### Features

* Add flow control for message publishing ([#96](https://www.github.com/googleapis/python-pubsub/issues/96)) ([06085c4](https://www.github.com/googleapis/python-pubsub/commit/06085c4083b9dccdd50383257799904510bbf3a0))


### Bug Fixes

* Fix PubSub incompatibility with api-core 1.17.0+ ([#103](https://www.github.com/googleapis/python-pubsub/issues/103)) ([c02060f](https://www.github.com/googleapis/python-pubsub/commit/c02060fbbe6e2ca4664bee08d2de10665d41dc0b))


### Documentation
- Clarify that Schedulers shouldn't be used with multiple SubscriberClients ([#100](#100)) ([cf9e87c](cf9e87c))
- Fix update subscription/snapshot/topic samples ([#113](#113)) ([e62c38b](e62c38b))


### Internal / Testing Changes
- Re-generated service implementaton using synth: removed experimental notes from the RetryPolicy and filtering features in anticipation of GA, added DetachSubscription (experimental) ([#114](#114)) ([0132a46](0132a46))
- Incorporate will_accept() checks into publish() ([#108](#108)) ([6c7677e](6c7677e))
@hongalex hongalex mentioned this pull request Nov 14, 2020
Closed
@quartzmo quartzmo mentioned this pull request May 14, 2021
Merged
@release-please release-please bot mentioned this pull request Aug 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kamalaboulhosn kamalaboulhosn kamalaboulhosn approved these changes

@pradn pradn pradn approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Milestone
No milestone
4 participants
@plamut @pradn @kamalaboulhosn @googlebot