-
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
Deprecate vttablet metrics QueryCacheXX
and rename to QueryPlanCacheXX
#16289
Changes from 3 commits
9adb889
6cf772d
ecc3210
16a6412
f23888d
623dd6b
c0230d8
ade9210
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,7 +99,6 @@ hotRowProtection: | |
consolidator: enable|disable|notOnPrimary # enable-consolidator, enable-consolidator-replicas | ||
passthroughDML: false # queryserver-config-passthrough-dmls | ||
streamBufferSize: 32768 # queryserver-config-stream-buffer-size | ||
queryCacheSize: 5000 # queryserver-config-query-cache-size | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This flag was removed a while ago, I took the liberty of removing this from the config file. |
||
schemaReloadIntervalSeconds: 1800 # queryserver-config-schema-reload-time | ||
watchReplication: false # watch_replication_stream | ||
terseErrors: false # queryserver-config-terse-errors | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -197,26 +197,26 @@ func TestQueryPlanCache(t *testing.T) { | |
ctx := context.Background() | ||
logStats := tabletenv.NewLogStats(ctx, "GetPlanStats") | ||
|
||
initialHits := qe.queryCacheHits.Get() | ||
initialMisses := qe.queryCacheMisses.Get() | ||
Comment on lines
-200
to
-201
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In order to ensure that we keep testing the old metrics until we delete them, we should keep the tests for them. That will require keeping the struct members also around until the metrics are deleted. Each vttablet has one instance of the queryEngine, so it should not be a big deal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done via ade9210 |
||
initialHits := qe.queryPlanCacheHits.Get() | ||
initialMisses := qe.queryPlanCacheMisses.Get() | ||
|
||
firstPlan, err := qe.GetPlan(ctx, logStats, firstQuery, false) | ||
require.NoError(t, err) | ||
require.NotNil(t, firstPlan, "plan should not be nil") | ||
|
||
assertPlanCacheSize(t, qe, 1) | ||
|
||
require.Equal(t, int64(0), qe.queryCacheHits.Get()-initialHits) | ||
require.Equal(t, int64(1), qe.queryCacheMisses.Get()-initialMisses) | ||
require.Equal(t, int64(0), qe.queryPlanCacheHits.Get()-initialHits) | ||
require.Equal(t, int64(1), qe.queryPlanCacheMisses.Get()-initialMisses) | ||
|
||
secondPlan, err := qe.GetPlan(ctx, logStats, firstQuery, false) | ||
require.NoError(t, err) | ||
require.NotNil(t, secondPlan, "plan should not be nil") | ||
|
||
assertPlanCacheSize(t, qe, 1) | ||
|
||
require.Equal(t, int64(1), qe.queryCacheHits.Get()-initialHits) | ||
require.Equal(t, int64(1), qe.queryCacheMisses.Get()-initialMisses) | ||
require.Equal(t, int64(1), qe.queryPlanCacheHits.Get()-initialHits) | ||
require.Equal(t, int64(1), qe.queryPlanCacheMisses.Get()-initialMisses) | ||
|
||
qe.ClearQueryPlanCache() | ||
} | ||
|
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.
The reason metric names are not prefixed with
Tablet
orGate
is that the binary name is automatically added to the metric name as a prefix. So this becomesvttablet_query_plan_cache_misses
if you look at/metrics
.All of the metrics introduced in this PR need to be renamed and the
Tablet
prefix should be removed.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.
Maybe you just need to drop the last commit!
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.
That won't work as the vtgate and vttablet metrics will have the same name in vtcombo, leading to a panic/failure.
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 will have to see how I can tweak vtcombo. We might already have this situation where the same name is used by both vtgate and vttablet in vtcombo.