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.

Sign up for GitHub

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

Jump to bottom

Implement polling tenants concurrently #3647

Merged
merged 22 commits into from
Aug 5, 2024
Merged

Implement polling tenants concurrently #3647

merged 22 commits into from
Aug 5, 2024

Conversation

zalegrala
Copy link
Contributor

@zalegrala zalegrala commented May 3, 2024 •
edited
Loading

What this PR does:

Here we implement basic polling for tenants concurrently. This is another step
towards making the polling more efficient. The next step will be to implement
a weighted polling strategy to poll tenants by some priority. For now, I think
this is a reasonable start and should stand on its own.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@github-actions github-actions bot added the stale Used for stale issues / PRs label Jul 3, 2024
@zalegrala zalegrala added keepalive Label to exempt Issues / PRs from stale workflow and removed stale Used for stale issues / PRs labels Jul 3, 2024
@grafana grafana deleted a comment from github-actions bot Jul 12, 2024
@zalegrala
Copy link
Contributor Author

I'm aware of a race in the tests when we check the error message if we exceed the threshold during polling. Since now we poll concurrently, we can't know which error will be the final error, and so an adjustment needs to be made to assert the error is one of n, rather than one specific. I'll follow up.

In the meantime, feedback welcome.

My thought to follow this up this PR is to modify the tenant loop, so we loop over tenants which are sorted by some metric, like block size, or last poll duration. This way we can start work on the ones that require the most effort first.

joe-elliott
joe-elliott reviewed Jul 15, 2024
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

this lgtm. not approving b/c it's still marked draft

tempodb/blocklist/poller.go Outdated Show resolved Hide resolved
@zalegrala zalegrala marked this pull request as ready for review July 15, 2024 13:10
@zalegrala
Copy link
Contributor Author

I've modified the error handling to be consistent, which required keeping of a []error for the duration of the Do() call. This allows us to know if the sequence of errors matches the sequence of the tenants. This might be less important than the order in which the errors arrive, but at least its consistent.

@zalegrala zalegrala force-pushed the tenantPollMulti branch 2 times, most recently from 9ad2b78 to 8a99b99 Compare July 17, 2024 15:16
joe-elliott
joe-elliott reviewed Jul 17, 2024
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

looking good. one thought.

can we add the new config option to the docs?

tempodb/blocklist/poller.go Outdated Show resolved Hide resolved
joe-elliott
joe-elliott reviewed Jul 31, 2024
View reviewed changes
tempodb/blocklist/poller.go Outdated Show resolved Hide resolved
tempodb/blocklist/poller.go Outdated Show resolved Hide resolved
tempodb/blocklist/poller.go Outdated Show resolved Hide resolved
joe-elliott
joe-elliott approved these changes Jul 31, 2024
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

let's go!

@zalegrala
Avoid race on consecutiveErrors
338e0a2
@zalegrala
Add comments for atomic use
1a1b423
@zalegrala
Increase tenant count, concurrency and randomness in poller integrati…
6d4fc0d 
…on test
@zalegrala
Drop test logs
1d78103
@zalegrala
Tidy up
8e6eb2b
@zalegrala
Include TenantPollConcurrency in deletion integration test
cece57c
@zalegrala
Match a regex on the error rather than a specifc one to account for t…
f167c6d 
…he concurrency
@zalegrala
Add log for base test
866d823
@zalegrala
Use math/rand/v2
922c8dc
@zalegrala
Fix for consistent error handling of consequetive errors
7fc79e0
@zalegrala
Reduce test work and output for truncation and timeouts
d125982
@zalegrala
Update changelog
6f533ef
@zalegrala
Adjust error handling
995a3cf 
Here we make changes to the error handling to account for the additional
complexity brought with the tenant concurrency.

This changes the behavior of the
blocklist_poll_tolerate_consecutive_errors configuration by applying to
a single tenant, which instructs the poller to retry until the threshold
is met.

A new configuration parameter blocklist_poll_tolerate_tenant_failures
has been added to account for the number of failing tenants that will be
tolerated.

This allows parts of the old behavior scoped to a single tenant, but
also accounts for a more global picture.  This means that a single
failing tenant by default will not stop the entire polling process.

Tests have been updated to account for this additional logic.
@zalegrala
Fix tenant failure increment
49e09fc
@zalegrala
Fix race in test
0f5b5a4
@zalegrala
Lint
5dec827
@zalegrala
Avoid race for integer read/write
c376df8
@zalegrala
Clarify tracking of failing tenants
e4f5bac
@zalegrala
Play golf
2daab09
@zalegrala zalegrala merged commit d3a9caf into grafana:main Aug 5, 2024
17 checks passed
@zalegrala zalegrala deleted the tenantPollMulti branch August 5, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiermolinar javiermolinar javiermolinar left review comments

@joe-elliott joe-elliott joe-elliott approved these changes

@annanay25 annanay25 Awaiting requested review from annanay25

@mdisibio mdisibio Awaiting requested review from mdisibio mdisibio is a code owner

@mapno mapno Awaiting requested review from mapno mapno is a code owner

@yvrhdn yvrhdn Awaiting requested review from yvrhdn yvrhdn is a code owner

@electron0zero electron0zero Awaiting requested review from electron0zero electron0zero is a code owner

@ie-pham ie-pham Awaiting requested review from ie-pham ie-pham is a code owner

@stoewer stoewer Awaiting requested review from stoewer stoewer is a code owner

@knylander-grafana knylander-grafana Awaiting requested review from knylander-grafana knylander-grafana is a code owner

Assignees
No one assigned
Labels
keepalive Label to exempt Issues / PRs from stale workflow
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zalegrala @joe-elliott @javiermolinar