-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 Frontend: dynamic horizontal query sharding #5658
Merged
yeya24
merged 13 commits into
thanos-io:main
from
pedro-stanaka:feat/dynamic-horizontal-sharding
Sep 14, 2022
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
6ba6102
Adding new parameter for more dynamic horizontal query sharding
pedro-stanaka 71694e9
Update docs for query FE
pedro-stanaka 8ac0e94
Adding new parameters and testing validation
pedro-stanaka fb0ce35
Using new parameters instead of changing behavior for existing ones
pedro-stanaka 330d50a
Adding changelog entry for changes
pedro-stanaka 7e2416f
Fixing problem on config validation and adding more test cases
pedro-stanaka 2787302
Fixing linting
pedro-stanaka a3d0970
Change name of cli arguments and improve validation
pedro-stanaka 2228999
Fixing changelog entry
pedro-stanaka fc6038d
Final touches on docs
pedro-stanaka 894088c
Adding params names to changelog
pedro-stanaka 5a336ce
Fixing CR comments
pedro-stanaka 72f13d7
Adding additional check for minimal split interval
pedro-stanaka File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
// Copyright (c) The Thanos Authors. | ||
// Licensed under the Apache License 2.0. | ||
|
||
package queryfrontend | ||
|
||
import ( | ||
"fmt" | ||
"testing" | ||
"time" | ||
|
||
"github.com/thanos-io/thanos/internal/cortex/chunk/cache" | ||
"github.com/thanos-io/thanos/internal/cortex/querier/queryrange" | ||
"github.com/thanos-io/thanos/pkg/testutil" | ||
) | ||
|
||
func TestConfig_Validate(t *testing.T) { | ||
|
||
type testCase struct { | ||
name string | ||
config Config | ||
err string | ||
} | ||
|
||
testCases := []testCase{ | ||
{ | ||
name: "invalid query range options", | ||
config: Config{ | ||
QueryRangeConfig: QueryRangeConfig{ | ||
SplitQueriesByInterval: 10 * time.Hour, | ||
HorizontalShards: 10, | ||
MinQuerySplitInterval: 1 * time.Hour, | ||
MaxQuerySplitInterval: day, | ||
}, | ||
}, | ||
err: "split queries interval and dynamic query split interval cannot be set at the same time", | ||
}, | ||
{ | ||
name: "invalid parameters for dynamic query range split", | ||
config: Config{ | ||
QueryRangeConfig: QueryRangeConfig{ | ||
SplitQueriesByInterval: 0, | ||
HorizontalShards: 0, | ||
MinQuerySplitInterval: 1 * time.Hour, | ||
}, | ||
}, | ||
err: "min horizontal shards should be greater than 0 when query split threshold is enabled", | ||
}, | ||
{ | ||
name: "invalid parameters for dynamic query range split - 2", | ||
config: Config{ | ||
QueryRangeConfig: QueryRangeConfig{ | ||
SplitQueriesByInterval: 0, | ||
HorizontalShards: 10, | ||
MaxQuerySplitInterval: 0, | ||
MinQuerySplitInterval: 1 * time.Hour, | ||
}, | ||
}, | ||
err: "max query split interval should be greater than 0 when query split threshold is enabled", | ||
}, | ||
{ | ||
name: "invalid parameters for dynamic query range split - 3", | ||
config: Config{ | ||
QueryRangeConfig: QueryRangeConfig{ | ||
SplitQueriesByInterval: 0, | ||
HorizontalShards: 10, | ||
MaxQuerySplitInterval: 1 * time.Hour, | ||
MinQuerySplitInterval: 0, | ||
}, | ||
LabelsConfig: LabelsConfig{ | ||
DefaultTimeRange: day, | ||
}, | ||
}, | ||
err: "min query split interval should be greater than 0 when query split threshold is enabled", | ||
}, | ||
{ | ||
name: "valid config with caching", | ||
config: Config{ | ||
DownstreamURL: "localhost:8080", | ||
QueryRangeConfig: QueryRangeConfig{ | ||
SplitQueriesByInterval: 10 * time.Hour, | ||
HorizontalShards: 0, | ||
MaxQuerySplitInterval: 0, | ||
MinQuerySplitInterval: 0, | ||
ResultsCacheConfig: &queryrange.ResultsCacheConfig{ | ||
CacheConfig: cache.Config{}, | ||
Compression: "", | ||
CacheQueryableSamplesStats: false, | ||
}, | ||
}, | ||
LabelsConfig: LabelsConfig{ | ||
DefaultTimeRange: day, | ||
}, | ||
}, | ||
err: "", | ||
}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
t.Run(tc.name, func(t *testing.T) { | ||
err := tc.config.Validate() | ||
if tc.err != "" { | ||
testutil.NotOk(t, err) | ||
testutil.Equals(t, tc.err, err.Error()) | ||
} else { | ||
testutil.Ok(t, err) | ||
fmt.Println(err) | ||
} | ||
}) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change might be out of scope for this PR, but I wonder why we even have this constraint. @yeya24 any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was inherited from Cortex (only by looking at git history). 17343c16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, we don't allow negative split intervals.