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

Avoid polling in worker #4094

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Avoid polling in worker #4094

wants to merge 2 commits into from

Conversation

jlearman
Copy link

@jlearman jlearman commented Jun 24, 2024

Pull Request Template

Description

The polling architecture with 200 ms sleep in ProcessEventMessages() has two drawbacks:

  • it imposes a rate limit of 5 events per second for any input source
  • it imposes an average latency of 100 ms per event

The polling loop can be avoided using a goroutine per worker that posts incoming events to the messageStream channel, and pending on that channel in ProcessEventMessages().

A short messageStream queue guarantees fairness: no worker's latest event will be postponed for more than the length of the channel.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

The same changes have been tested in another project.

Additional Context (Please include any Screenshots/gifs if relevant)

...

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings
  • I have tagged the reviewers in a comment below incase my pull request is ready for a review
  • I have signed the commit message to agree to Developer Certificate of Origin (DCO) (to certify that you wrote or otherwise have the right to submit your contribution to the project.) by adding "--signoff" to my git commit command.

@jlearman jlearman requested a review from dabooz June 24, 2024 16:20
@dabooz dabooz requested a review from MaxMcAdam June 24, 2024 18:21
@jlearman
Copy link
Author

jlearman commented Jun 24, 2024

This is in Draft because

  • I'm new here
  • There aren't any UTs
  • It may need more testing/verification. These changes have been validated in a different project, and I'm confident they're good in that context. I don't know how to test this in context.

It's not Draft because I expect to add any changes, other than based on reviews & testing.

It should reduce request/response latency and improve throughput and scalability.

@jlearman jlearman requested a review from dlarson04 June 25, 2024 15:02
@jlearman jlearman force-pushed the avoid_worker_polling branch 2 times, most recently from 42e26a9 to 6265838 Compare June 26, 2024 14:17
The polling architecture with 200 ms sleep in ProcessEventMessages()
has two drawbacks:

- it imposes a rate limit of 5 events per second for any input source
- it imposes an average latency of 100 ms per event

The polling loop can be avoided using a goroutine per worker that
posts incoming events to the `messageStream` channel, and pending on
that channel in ProcessEventMessages().

A short `messageStream` queue guarantees fairness: no worker's latest
event will be postponed for more than the length of the channel.

Signed-off-by: Jeff Learman <[email protected]>
@jlearman jlearman force-pushed the avoid_worker_polling branch from 6265838 to d6fedda Compare June 26, 2024 15:42
@jlearman jlearman marked this pull request as ready for review July 24, 2024 12:53
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.

1 participant