*: specify time range via TIME_RANGE hint for metrics/inspection tables #14874
Conversation
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
ghost
requested review from
francis0407 and
alivxxx
and removed request for
a team
Signed-off-by: Lonng <heng@lonng.org>
|
/run-all-tests |
Signed-off-by: Lonng <heng@lonng.org>
lonng
changed the title
Signed-off-by: Lonng <heng@lonng.org>
Signed-off-by: Lonng <heng@lonng.org>
Deardrops
reviewed
Feb 21, 2020
executor/inspection_summary.go
Outdated
| return nil, meta.ErrTableNotExists | ||
| def, found := infoschema.MetricTableMap[name] | ||
| if !found { | ||
| sctx.GetSessionVars().StmtCtx.AppendWarning(fmt.Errorf("metrics table: %s not found34", name)) |
There was a problem hiding this comment.
Suggested change
| sctx.GetSessionVars().StmtCtx.AppendWarning(fmt.Errorf("metrics table: %s not found34", name)) | |
| sctx.GetSessionVars().StmtCtx.AppendWarning(fmt.Errorf("metrics table: %s not found", name)) |
| @@ -2739,7 +2746,40 @@ func (b *PlanBuilder) buildDataSource(ctx context.Context, tn *ast.TableName, as | |||
| return result, nil | |||
| } | |||
|
|
|||
| func (b *PlanBuilder) buildMemTable(ctx context.Context, dbName model.CIStr, tableInfo *model.TableInfo) (LogicalPlan, error) { | |||
| func (b *PlanBuilder) timeRangeForSummaryTable() QueryTimeRange { | |||
| const defaultSummaryDuration = 30 * time.Minute | |||
There was a problem hiding this comment.
Make this a global variable.
There was a problem hiding this comment.
It's not a good idea because this const will not share with other functions.
| } | ||
| e.LabelConditions = extractCols |
There was a problem hiding this comment.
Should we store extractCols to e.LabelConditions, even skipRequest is true?
For avoiding check e.LabelConditions == nil after use this Extract function
There was a problem hiding this comment.
The cols always nil if skipRequest==true.
Signed-off-by: Lonng <heng@lonng.org>
Signed-off-by: Lonng <heng@lonng.org>
|
LGTM |
|
/run-all-tests |
|
/run-common-test tidb-test=pr/992 |
|
/run-integration-common-test tidb-test=pr/992 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Signed-off-by: Lonng heng@lonng.org
What problem does this PR solve?
This PR refactors
inspection/metricssystem tables and improve internal consistency:instanceto an individual column from labels.metric_schematometrics_schema.TIME_RANGEhint to specify the time range for metrics summary tables instead of using a redundant columntime.What is changed and how it works?
Add a
QueryTimeRangefor memory table executor and give the hint to retrievers.Check List
Tests
Release note