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

[CHANGED] Count message headers toward subscription limits #583

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

kozlovic
Copy link
Member

When headers were introduced, the whole buffer (so data + hdr) was added as pending limits, but when messages were removed from the subscription, only the data part was removed, which caused an issue that was address in PR #420 by just accounting for the data size (not header).

Headers becoming more and more prevalent, it could be that a big part of message content is the headers, so we should probably count the headers size along with the data toward the subscription's bytes limit.

Signed-off-by: Ivan Kozlovic ivan@synadia.com

When headers were introduced, the whole buffer (so data + hdr) was
added as pending limits, but when messages were removed from the
subscription, only the data part was removed, which caused an
issue that was address in PR #420 by just accounting for the
data size (not header).

Headers becoming more and more prevalent, it could be that a big
part of message content is the headers, so we should probably
count the headers size along with the data toward the subscription's
bytes limit.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@kozlovic kozlovic merged commit 5b8f3aa into main Sep 19, 2022
@kozlovic kozlovic deleted the msg_hdr_data_len branch September 19, 2022 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants