-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(pubsub): add publisher flow control support #4292
Conversation
…into pubsub-publish-fc
…into pubsub-publish-fc
switch flowController to struct not pointer fix more comments fix more comments
c4b5270
to
c3c0141
Compare
pubsub/topic.go
Outdated
FlowControlSettings: FlowControlSettings{ | ||
MaxOutstandingMessages: 1000, | ||
MaxOutstandingBytes: 10 * MaxPublishRequestBytes, | ||
LimitExceededBehavior: FlowControlBlock, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With BufferedByteLimit, what was the behavior by default? Was it to block? Either way, we should probably make sure that we don't introduce blocking due to MaxOutstandingMessages
. That means we may either want to set MaxOutstandingMessages
effectively to infinity or set the LimitExceededBehavior to Ignore by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default behavior of BufferedByteLimit is returning an error. Yeah, I think in retrospect, MaxOutstandingMessages should be disabled by default. Right now, I have some logic determining if these values have been set by the user, but that's probably too complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should we change this in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the default for other languages? I defaulted to block because I thought that makes more sense, but consistency is probably more important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok it seems like the default is ignore for Java, and I think I'll stick with this.
pubsub/topic.go
Outdated
FlowControlSettings: FlowControlSettings{ | ||
MaxOutstandingMessages: 1000, | ||
MaxOutstandingBytes: 10 * MaxPublishRequestBytes, | ||
LimitExceededBehavior: FlowControlBlock, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should we change this in that case?
This PR adds publisher flow control support, similar to the subscriber flow control. This is different in that the settings are part of
topic.PublishSettings.FlowControlSettings
. Similar to the subscriber flow control, these settings are locked-in on the firstPublish
call and subsequent modifications will not have any effect. In theFlowControlSettings.LimitExceededBehavior = FlowControlSignalError
mode, oversized messages will result in an error from callingpublishResult.Get
, but oversized messages in other modes (Block/Ignore), will treat the message as the largest possible message size.FlowControlSettings.MaxOutstandingBytes
has a small overlap with the existingPublishSettings.BufferedByteLimit
, which specifies an upper bound of bytes to keep in memory before throwing an error. This PR addresses this by adding a deprecation notice toBufferedByteLimit
, but both will continue to be supported until a major version release. IfMaxOutstandingBytes
is passed in, it will overrideBufferedByteLimit
.The integration tests are largely based on the Java implementation. One difference between this and the Ruby/Python/Java implementations is that the semaphores enforce the queueing system. Specifically, large acquires will stay at head of line which avoids deadlocking.
Fixes #3210