Query Frontend: dynamic horizontal query sharding #5658

pedro-stanaka · 2022-08-30T08:19:23Z

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Summary

This is a possible solution for the issue #5617.
Closes #5617.

Changes

Adds new CLI parameter for QFE to allow a lower boundary for query horizontal split
When queries have a duration above this threshold, they will be split into either a minimum number of horizontal shards or an upper bound interval (for longer queries).

Verification

The modification was only on the Query Range middleware, I introduced more test cases to test the new behavior.
I also added more validation for new parameters and tests to cover it.

pkg/queryfrontend/config_test.go

CHANGELOG.md

fpetkovski

Looks good overall, I think we just need to polish validation a bit better

cmd/thanos/query_frontend.go

pkg/queryfrontend/config.go

fpetkovski

Just a few more documentation nits, everything else looks good 👍

cmd/thanos/query_frontend.go

fpetkovski · 2022-09-01T08:42:04Z

pkg/queryfrontend/config.go

@@ -242,14 +245,25 @@ type LabelsConfig struct {
 // Validate a fully initialized config.
 func (cfg *Config) Validate() error {
 	if cfg.QueryRangeConfig.ResultsCacheConfig != nil {
-		if cfg.QueryRangeConfig.SplitQueriesByInterval <= 0 {
-			return errors.New("split queries interval should be greater than 0 when caching is enabled")
+		if cfg.QueryRangeConfig.SplitQueriesByInterval <= 0 && !cfg.isDynamicSplitSet() {


This change might be out of scope for this PR, but I wonder why we even have this constraint. @yeya24 any ideas?

I think this was inherited from Cortex (only by looking at git history). 17343c16

Right, we don't allow negative split intervals.

fpetkovski

Awesome feature, lgtm 👍

@yeya24 any other comments?

yeya24 · 2022-09-09T15:43:10Z

CHANGELOG.md

@@ -17,6 +17,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
 ### Added
 * [#5654](https://github.com/thanos-io/thanos/pull/5654) Query: add `--grpc-compression` flag that controls the compression used in gRPC client. With the flag it is now possible to compress the traffic between Query and StoreAPI nodes - you get lower network usage in exchange for a bit higher CPU/RAM usage.
 - [#5650](https://github.com/thanos-io/thanos/pull/5650) Query Frontend: Add sharded queries metrics.
+- [#5658](https://github.com/thanos-io/thanos/pull/5658) Query Frontend: Introduce new parameters to implement more dynamic horizontal query splitting.


Can you mention the new flags name here?

yeya24 · 2022-09-09T15:44:23Z

cmd/thanos/query_frontend.go

+		"One should also set query-range.split-min-horizontal-shards to a value greater than 1 to enable splitting.").
+		Default("0").DurationVar(&cfg.QueryRangeConfig.MinQuerySplitInterval)
+
+	cmd.Flag("query-range.max-split-interval", "Split query range bellow this interval in query-range.horizontal-shards. Queries with a range longer than this value will be split in multiple requests of this length.").


type below

yeya24 · 2022-09-09T15:45:01Z

pkg/queryfrontend/config.go

-		if cfg.QueryRangeConfig.SplitQueriesByInterval <= 0 {
-			return errors.New("split queries interval should be greater than 0 when caching is enabled")
+		if cfg.QueryRangeConfig.SplitQueriesByInterval <= 0 && !cfg.isDynamicSplitSet() {
+			return errors.New("split queries  or split threshold interval should be greater than 0 when caching is enabled")


Can we remove 1 space?

yeya24 · 2022-09-09T15:45:27Z

pkg/queryfrontend/config.go

+
+	if cfg.isDynamicSplitSet() {
+		err := cfg.validateDynamicSplitParams()
+		if err != nil {


We can use if err := xxx; err != nil {} pattern

yeya24 · 2022-09-09T15:48:31Z

pkg/queryfrontend/roundtrip_test.go

+			handlerFunc:   promqlResults,
+			codec:         queryRangeCodec,
+			splitInterval: 1 * time.Hour,
+			expected:      2,


This test case is the same as the same at line 279?

Good catch, thank you!

yeya24 · 2022-09-09T15:52:14Z

pkg/queryfrontend/roundtrip.go

+		}
+
+		queryInterval := time.Duration(r.GetEnd()-r.GetStart()) * time.Millisecond
+		// if the query is multiple of max interval, we use the max interval to split.


Let's capitalize the comment.

yeya24 · 2022-09-09T15:52:19Z

pkg/queryfrontend/roundtrip.go

+			return config.MaxQuerySplitInterval
+		}
+
+		// if the query duration is less than max interval, we split it equally in HorizontalShards.


yeya24 · 2022-09-09T15:53:00Z

pkg/queryfrontend/roundtrip.go

+		}
+
+		// if the query duration is less than max interval, we split it equally in HorizontalShards.
+		return time.Duration(queryInterval.Milliseconds()/config.HorizontalShards) * time.Millisecond


I don't see where MinQuerySplitInterval is used here. We need to also ensure the query interval is larger than MaxQuerySplitInterval IIUC?

MinQuerySplitInterval is a drop-in replacement for SplitQueriesByInterval, saying just when we start splitting queries. We use it in newQueryRangeTripperware (line 182) to decide whether to send split requests or to send the original request.

Regarding the MaxQuerySplitInterval, we are checking that on line 247 to see if the query interval is at least twice the size of the MaxQuerySplitInterval, if so we use that as our split interval.

MinQuerySplitInterval is a drop-in replacement for SplitQueriesByInterval, saying just when we start splitting queries. We use it in newQueryRangeTripperware (line 182) to decide whether to send split requests or to send the original request.

Where do we ensure that the query range is larger than the MinQuerySplitInterval to be split horizontally? For this code, I can see that even a small range query will be sharded by HorizontalShards times.
Maybe I misunderstand the initial design.

Yeah, you are right. I added new test case and an if statement.

Signed-off-by: Pedro Tanaka <[email protected]>

yeya24

LGTM

pull-request-size bot added the size/M label Aug 30, 2022

pedro-stanaka force-pushed the feat/dynamic-horizontal-sharding branch 2 times, most recently from afabd95 to bc57bc6 Compare August 30, 2022 09:41

pull-request-size bot added size/L and removed size/M labels Aug 30, 2022

pedro-stanaka changed the title ~~Adding new parameter for more dynamic horizontal query sharding~~ Query Frontend: dynamic horizontal query sharding Aug 30, 2022

sonatype-lift bot reviewed Aug 30, 2022

View reviewed changes

pkg/queryfrontend/config_test.go Outdated Show resolved Hide resolved

pedro-stanaka force-pushed the feat/dynamic-horizontal-sharding branch from 941ae32 to 9eddb3d Compare August 30, 2022 19:33

yeya24 reviewed Aug 30, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pedro-stanaka marked this pull request as ready for review August 31, 2022 07:03

fpetkovski reviewed Aug 31, 2022

View reviewed changes

cmd/thanos/query_frontend.go Outdated Show resolved Hide resolved

cmd/thanos/query_frontend.go Outdated Show resolved Hide resolved

pkg/queryfrontend/config.go Outdated Show resolved Hide resolved

pedro-stanaka force-pushed the feat/dynamic-horizontal-sharding branch 3 times, most recently from 41171f2 to f4c6fb8 Compare September 1, 2022 07:38

fpetkovski reviewed Sep 1, 2022

View reviewed changes

pedro-stanaka force-pushed the feat/dynamic-horizontal-sharding branch from bd6417e to 9484d0b Compare September 8, 2022 15:17

fpetkovski previously approved these changes Sep 9, 2022

View reviewed changes

yeya24 requested changes Sep 9, 2022

View reviewed changes

pedro-stanaka dismissed fpetkovski’s stale review via 3ebf35b September 12, 2022 09:43

pedro-stanaka force-pushed the feat/dynamic-horizontal-sharding branch from 9484d0b to 3ebf35b Compare September 12, 2022 09:43

pedro-stanaka added 10 commits September 13, 2022 10:02

Adding new parameter for more dynamic horizontal query sharding

6ba6102

Signed-off-by: Pedro Tanaka <[email protected]>

Update docs for query FE

71694e9

Signed-off-by: Pedro Tanaka <[email protected]>

Adding new parameters and testing validation

8ac0e94

Signed-off-by: Pedro Tanaka <[email protected]>

Using new parameters instead of changing behavior for existing ones

fb0ce35

Signed-off-by: Pedro Tanaka <[email protected]>

Adding changelog entry for changes

330d50a

Signed-off-by: Pedro Tanaka <[email protected]>

Fixing problem on config validation and adding more test cases

7e2416f

Signed-off-by: Pedro Tanaka <[email protected]>

Fixing linting

2787302

Signed-off-by: Pedro Tanaka <[email protected]>

Change name of cli arguments and improve validation

a3d0970

Signed-off-by: Pedro Tanaka <[email protected]>

Fixing changelog entry

2228999

Signed-off-by: Pedro Tanaka <[email protected]>

Final touches on docs

fc6038d

Signed-off-by: Pedro Tanaka <[email protected]>

pedro-stanaka added 3 commits September 13, 2022 10:02

Adding params names to changelog

894088c

Signed-off-by: Pedro Tanaka <[email protected]>

Fixing CR comments

5a336ce

Signed-off-by: Pedro Tanaka <[email protected]>

Adding additional check for minimal split interval

72f13d7

Signed-off-by: Pedro Tanaka <[email protected]>

pedro-stanaka force-pushed the feat/dynamic-horizontal-sharding branch from 3ebf35b to 72f13d7 Compare September 13, 2022 08:03

yeya24 approved these changes Sep 13, 2022

View reviewed changes

yeya24 merged commit 0392dd7 into thanos-io:main Sep 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query Frontend: dynamic horizontal query sharding #5658

Query Frontend: dynamic horizontal query sharding #5658

pedro-stanaka commented Aug 30, 2022 •

edited

Loading

fpetkovski left a comment

fpetkovski left a comment

fpetkovski Sep 1, 2022

pedro-stanaka Sep 1, 2022

yeya24 Sep 9, 2022

fpetkovski left a comment

yeya24 Sep 9, 2022

yeya24 Sep 9, 2022

yeya24 Sep 9, 2022

yeya24 Sep 9, 2022

yeya24 Sep 9, 2022

pedro-stanaka Sep 12, 2022

yeya24 Sep 9, 2022

yeya24 Sep 9, 2022

yeya24 Sep 9, 2022

pedro-stanaka Sep 12, 2022

yeya24 Sep 12, 2022 •

edited

Loading

pedro-stanaka Sep 13, 2022

yeya24 left a comment

Query Frontend: dynamic horizontal query sharding #5658

Query Frontend: dynamic horizontal query sharding #5658

Conversation

pedro-stanaka commented Aug 30, 2022 • edited Loading

Summary

Changes

Verification

fpetkovski left a comment

Choose a reason for hiding this comment

fpetkovski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpetkovski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeya24 Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeya24 left a comment

Choose a reason for hiding this comment

pedro-stanaka commented Aug 30, 2022 •

edited

Loading

yeya24 Sep 12, 2022 •

edited

Loading