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

Query all ingesters to mitigate 404s on rollout #557

Merged
merged 8 commits into from
Mar 2, 2021

Conversation

annanay25
Copy link
Contributor

@annanay25 annanay25 commented Feb 25, 2021

Signed-off-by: Annanay [email protected]

What this PR does:
This is an experimental PR to check if querying all ingesters helps mitigate 404s on rollouts/scaleups. Will also be monitoring performance as it queries a lot more ingesters every time.
The PR also simplifies the sharding logic and adds a QueryMode at the Querier. This can be set to "ingesters" / "blocks" / "all" (default) and will only query the specified components.

Notez:

  • after testing it in our internal environment, we did not see signs of a performance penalty, ingesters respond queriers quickly
  • we need minimum of 2 shards as one is reserved for querying ingesters

Which issue(s) this PR fixes:
Attempts to fix #284 #38

Checklist

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

@annanay25 annanay25 marked this pull request as ready for review February 26, 2021 13:38
cmd/tempo/app/modules.go Show resolved Hide resolved
modules/frontend/querysharding.go Outdated Show resolved Hide resolved
modules/querier/http.go Outdated Show resolved Hide resolved
modules/querier/querier.go Show resolved Hide resolved
@@ -63,15 +66,15 @@ func (s shardQuery) Do(r *http.Request) (*http.Response, error) {
}

reqs := make([]*http.Request, s.queryShards)
for i := 0; i < len(s.blockBoundaries)-1; i++ {
for i := 0; i < s.queryShards; i++ {
Copy link
Member

Choose a reason for hiding this comment

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

instead of creating block boundaries, it would be a lot cleaner to make a method that created a slice of structs that represented the query. like:

type queryParams struct {
  queryShards bool
  startBlock []byte
  endBlock []byte
}

that would simplify this logic quite a bit and would be much easier to test as well.

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.

Adding an approval b/c I think this is basically done, but there are a few nits.

pkg/tempopb/tempo.proto Outdated Show resolved Hide resolved
modules/querier/http.go Show resolved Hide resolved
modules/querier/http.go Outdated Show resolved Hide resolved
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.

404s Temporarily returned when rolling ingesters
2 participants