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

Soft treshhold for the max batch size in bytes #269

Merged
merged 4 commits into from
Apr 16, 2023

Conversation

AfonsoRibeiro
Copy link
Contributor

If this threshold is reached before batch size the batch is considered full and sent.
A soft threshold means there is no performance penalty with this implementation

#268

@tisonkun tisonkun self-requested a review April 12, 2023 08:58
Signed-off-by: tison <wander4096@gmail.com>
Copy link
Contributor

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

I notice that the Java client handle CommandConnected event and use maxMessageSize also. Perhaps we can have a follow-up to handle it also.

Moreover, we always check full after queue the new message so it's a threshold than a guard, while we may check size before actually push_back.

@tisonkun
Copy link
Contributor

cc @FlorentinDUBOIS @BewareMyPower for peer reviewing.

@AfonsoRibeiro I'll appreciate it if you can add a function test.

Copy link
Collaborator

@FlorentinDUBOIS FlorentinDUBOIS left a comment

Choose a reason for hiding this comment

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

Seems good to me

@tisonkun
Copy link
Contributor

Merging...

Potential improvements mentioned above can be follow-ups.

Thanks a lot for your contribution @AfonsoRibeiro!

@tisonkun tisonkun merged commit bd590e0 into streamnative:master Apr 16, 2023
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.

5 participants