-
Notifications
You must be signed in to change notification settings - Fork 146
feat(buffer_memory): allow buffer spillover #510
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(buffer_memory): allow buffer spillover #510
Conversation
e4ec353 to
16fe1d0
Compare
internal/impl/pure/buffer_memory.go
Outdated
|
|
||
| for (m.bytes + extraBytes) > m.cap { | ||
| if m.spilloverEnabled { | ||
| return component.ErrMessageTooLarge |
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.
We can introduce a new error type for spillover, if that makes sense.
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.
Yeah I think it would be better to add a different error message - will be easier to debug too.
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.
Done in e36405d and rebased.
|
@gregfurman @jem-davies Any thoughts on this approach? I'm happy to make any changes. |
Adds the 'spillover' boolean field to the 'memory' buffer component (default false). When true, any incoming message batch that will put the buffer above its configured 'limit' will be dropped. The motivation for this is to support input sources that do not gracefully handle backpressure and where the desired delivery guarantees are best effort.
16fe1d0 to
2dafcb5
Compare
2dafcb5 to
e36405d
Compare
|
Fixed the tests |
|
@alecmerdler 👋 Thanks for the contribution! While I'll give this proper review a bit later today, this PR got thinking whether the buffer:
memory:
limit: 1000
drop_on:
- check: this.errored()
- check: this == ""
# etc ...Could be over-engineering but am curious to hear your thoughts 🤔 |
|
Adding conditional buffering does seem like overkill and starts to mix concerns. What's missing for my use case is having any layer between inputs and buffers where we could drop on backpressure. Either way, since the |
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.
LGTM. Will give some more thought on how we can extend the buffer component but agree adding this spillover flag makes sense 👍
Adds the
spilloverboolean field to thememorybuffer component (default false). When true, any incoming message batch that will put the buffer above its configuredlimitwill be dropped.The motivation for this is to support input sources that do not gracefully handle backpressure and where the desired delivery guarantees are best effort.
Resolves #509