Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: add global consume timeout#1061

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
bojan-rajkovic-simplisafe wants to merge9 commits intoBlizzard:master
base:master
Choose a base branch
Loading
frombojan-rajkovic-simplisafe:brajkovic/feat/add-global-consume-timeout

Conversation

@bojan-rajkovic-simplisafe

This solves a similar problem to#1052, but does it with an approach that I think is a little cleaner and doesn't change existing consumer behavior — even if the existing consumer behavior is arguably wrong, all behavioral changes are susceptible to thespacebar heating problem.

Rather than making the specified timeout both the single-message consume timeoutand the batch timeout, it introduces a new value called the total consume timeout, which controls how long we can spend trying to fill a batch ofn messages with theconsume(num, cb) variant, and checks it on every iteration of the consume loop.

I did borrow the tests from that PR though, as they provided a pretty elegant framework to check the approach.

…consumenum workerthe consumenum worker consumes up to a certain # of messages, and onlyapplies the configured timeout to a per-message consumption, breakingout of the consume loop when:1. the end of the partition is reached (if partition eof messages areenabled)2. a consume times out3. the batch size is filledthis leaves the worker susceptible to slow-loris type issues, wheremessages come in at a cadence *just* under the timeout (e.g. every 900milliseconds) -- with a batch size of 1000, that means that`consumer.consume(1000, 1000, cb)` could take as many as 900 secondsto complete, which is less than ideal.introduce a configuration in the consumenum worker that allows callersto specify a "total" timeout that gets applied to the entire operation-- when exceeded, we break out of the loop and return the batch, nomatter how many messages were read.
…++-landthis builds on the previous commit by:1. implementing the total consume timeout parameter on the consumemethod's binding2. cleaning up the argument conversions a little bit to make themcleaner to read -- instead of deciding whether we're in thetwo-argument consume(timeout, cb) variant or the newly-4-argumentconsume(timeout, totalTimeout, num, cb) variant based on the type ofthe 2nd argument, it instead looks at the # of arguments
builds on top of the previous two commits and threads the totalconsume timeout value into javascript-land. the default is`undefined`, as that will result in no behavioral changes forconsumers. the new behavior is thus entirely opt-in.
@bojan-rajkovic-simplisafe
Copy link
Author

e2e is failing, appears to be related to the argument checking — I suspect I either didn’t rebuild the integration fully or messed up a check somewhere…

@bojan-rajkovic-simplisafebojan-rajkovic-simplisafe marked this pull request as draftDecember 6, 2023 22:12
@bojan-rajkovic-simplisafebojan-rajkovic-simplisafe marked this pull request as ready for reviewDecember 7, 2023 02:56
@bojan-rajkovic-simplisafe
Copy link
Author

Fixed — in the process, ended up cleaning up C++-land a bit and killing some dead code + some code that I'm not sure I understand how it ever worked. :)

@bojan-rajkovic-simplisafe
Copy link
Author

@GaryWilber@iradul It might be good to get one of you to look at both this and#1053 and decide which approach to take, merge one, and do some testing before cutting a release. I'm fairly confident in mine, but it could probably use more testing regardless…

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@bojan-rajkovic-simplisafe

[8]ページ先頭

©2009-2025 Movatter.jp