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

Repeaters, v.2: Sleep 0.2s if all locked #35686

Closed
wants to merge 6 commits into from
Closed

Conversation

kaapstorm
Copy link
Contributor

@kaapstorm kaapstorm commented Jan 27, 2025

Technical Summary

This past weekend I was (over?)thinking about how PR #35033 loops over repeaters, and I came up with two alternatives. This "version 2" is the first. (See PR #35687 for version 3.)

Comparison

PR #35033 Version 2 Version 3
Finds new repeat records every 5 minutes 1 minute 1 minute
Queries for all repeaters every 5 minutes After 200ms As soon as needed
Rank in order of simplicity 1 2 3
Rank in order of throughput 3 2 1

The approach used in PR #35033 is relatively simple, and fast at sending repeat records of repeaters that are already being processed. But new repeat records of repeaters that are not being processed can take up to 5 minutes to be discovered and processed.

In comparison, this version, version 2, checks every minute for repeaters with repeat records ready to be sent (and if it is busy processing repeaters, it will find new repeat records sooner than that).

If all repeaters are locked, it will wait 200ms for a repeater to finish processing, and then query for repeaters again. This approach is simpler than the approach taken in version 3, but it can result in queries returning all repeaters locked. It is this unnecessary churn on Postgres that prompted version 3.

Here is what version 2 looks like in Datadog:

image

Review commits together 🏢

Feature Flag

PROCESS_REPEATERS

Safety Assurance

Safety story

  • Tested on Staging
  • Not yet in use on Production

Automated test coverage

Tests included

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@kaapstorm kaapstorm added the product/invisible Change has no end-user visible impact label Jan 27, 2025
@kaapstorm
Copy link
Contributor Author

I'm not sure that the increase in performance makes up for the increase in complexity. Closing.

@kaapstorm kaapstorm closed this Feb 14, 2025
@kaapstorm kaapstorm deleted the nh/iter_repeaters_2 branch February 15, 2025 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants