Query Frontend For Logical Query Plan Support #6884
Conversation
rubywtl
force-pushed
the
frontend/middleware-logicalplan-support
branch
from
5e6383f to
3631c26
Compare
…ed execution feature flag Signed-off-by: rubywtl <[email protected]>
rubywtl
force-pushed
the
frontend/middleware-logicalplan-support
branch
from
7c03859 to
0d12680
Compare
pkg/querier/tripperware/instantquery/logical_plan_gen_instant_test.go
Outdated
Show resolved
Hide resolved
…tests Signed-off-by: rubywtl <[email protected]>
rubywtl
force-pushed
the
frontend/middleware-logicalplan-support
branch
from
5b66cc6 to
22df406
Compare
pkg/querier/tripperware/instantquery/logical_plan_gen_instant_test.go
Outdated
Show resolved
Hide resolved
…lan-gen middleware helper functions Signed-off-by: rubywtl <[email protected]>
Signed-off-by: rubywtl <[email protected]>
rubywtl
force-pushed
the
frontend/middleware-logicalplan-support
branch
from
24b8251 to
a278837
Compare
| ) | ||
|
|
||
| const ( | ||
| distributedExecEnabled = false |
There was a problem hiding this comment.
nit: I think this constant is decreasing the readability of the code. If you wanted to indicate the new parameter for this, maybe you could have a local variable or just add a comment:
distributedExecEnabled := false
instantQueryMiddleware, err := Middlewares(
log.NewNopLogger(),
mockLimitsShard{maxQueryLookback: 2},
nil,
qa,
5*time.Minute,
distributedExecEnabled,
)
instantQueryMiddleware, err := Middlewares(
log.NewNopLogger(),
mockLimitsShard{maxQueryLookback: 2},
nil,
qa,
5*time.Minute,
false , // distributedExecEnabled
)
| // Integration test for query round-trip with distributed execution enabled (feature flag). | ||
| // Checks logical plan is generated, included in request body, and processed correctly. | ||
|
|
||
| func TestRoundTripWithDistributedExec(t *testing.T) { |
There was a problem hiding this comment.
Instead of having three separate test functions, can we merge them into one with multiple test cases? A lot of code seem to be repeating.
| } | ||
|
|
||
| // TestRangeLogicalPlan validates the range logical plan generation middleware. | ||
| func TestRangeLogicalPlan(t *testing.T) { |
There was a problem hiding this comment.
I think we can merge this into the above test, the only difference seem to be start and stop params of prometheus requests.
| // Test: Execute middleware to populate the logical plan | ||
| _, err := handler.Do(context.Background(), tc.input) | ||
| require.NoError(t, err) | ||
| require.NotEmpty(t, tc.input.LogicalPlan, "prom request should not be empty") |
There was a problem hiding this comment.
Instead of just checking emptiness, is there a way to test type or content of the logical plan?
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Should we consider to fallback to non-distributed exec without failing the reqeust with a warning message? Just in case a bug introduced in logical planner doesn't take down the whole query availability.
| @@ -25,13 +26,17 @@ var ( | |||
| ShardedPrometheusCodec = NewPrometheusCodec(false, "", "protobuf") | |||
| ) | |||
|
|
|||
| const ( | |||
| distributedExecEnabled = false | |||
There was a problem hiding this comment.
Same comment as above, this const is better to live locally or as a comment for better readability
| } | ||
| } | ||
|
|
||
| func TestRoundTripWithoutDistributedExec(t *testing.T) { |
There was a problem hiding this comment.
Same as above comment, let's merge them into single function and just have different test cases
Signed-off-by: rubywtl <[email protected]>
What this PR does:
This PR adds logical plan generation support to the query frontend as part of the distributed query execution implementation.
Features
Configuration Changes
Testing Coverage
Integration Tests
Unit Tests
Which issue(s) this PR fixes:
Related to #6789
Checklist
CHANGELOG.mdupdated