--consolidator-query-waiter-cap to set the max number of waiter for consolidated query #17244

jwangace · 2024-11-16T16:29:18Z

Description

This PR proposed to use config ConsolidatorQueryWaiterCap to set the max number of clients Wait() for all consolidated queries, more same queries will fall back to use getConn() where is protected by connection pools.

With configureable ConsolidatorQueryWaiterCap (default to 0 means unlimit, user can adjust this value accordingly), when properly set would help to prevent when the Result of first query been returned, same Result would be sent concurrently to large number of clients, cause memory abruptly went high.

Related Issue(s)

Issue: #17243
Documentation: vitessio/website#1900

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

vitess-bot · 2024-11-16T16:29:21Z

go/vt/vttablet/tabletserver/tabletenv/config.go

harshit-gangal

This would also require addition to summary notes for v22 over https://github.com/vitessio/vitess/blob/main/changelog/22.0/22.0.0/summary.md

You can see v21 summary notes to see the template to add new configuration to release notes

harshit-gangal · 2024-11-20T14:09:57Z

Website docs would also need an update https://vitess.io/docs/22.0/user-guides/configuration-advanced/query-consolidation/

jwangace · 2024-11-21T01:01:51Z

This would also require addition to summary notes for v22 over https://github.com/vitessio/vitess/blob/main/changelog/22.0/22.0.0/summary.md

You can see v21 summary notes to see the template to add new configuration to release notes

Updated.

jwangace · 2024-11-21T01:02:35Z

Website docs would also need an update https://vitess.io/docs/22.0/user-guides/configuration-advanced/query-consolidation/

Hi @harshit-gangal This is not in this repo, is it?

codecov · 2024-11-21T01:18:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.51%. Comparing base (45192d2) to head (e5cdf1a).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17244      +/-   ##
==========================================
- Coverage   67.52%   67.51%   -0.02%     
==========================================
  Files        1581     1581              
  Lines      253948   253958      +10     
==========================================
- Hits       171480   171454      -26     
- Misses      82468    82504      +36

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jwangace · 2024-11-30T17:11:59Z

This can lead to slowness in query serving path. Either we make it an absolute count than per query count or we add a benchmark test for it.

Pushed new solution to use absolute count.

harshit-gangal · 2024-12-03T11:24:20Z

go/vt/vttablet/tabletserver/tabletenv/config.go

@@ -1014,6 +1016,7 @@ var defaultConfig = TabletConfig{
 	Consolidator:                Enable,
 	ConsolidatorStreamTotalSize: 128 * 1024 * 1024,
 	ConsolidatorStreamQuerySize: 2 * 1024 * 1024,
+	ConsolidatorQueryWaiterCap:  0,


nit: this can be ignored to be kept in default config for 0 as default

I dropped this line and use 0 as default in the flag.

harshit-gangal · 2024-12-03T11:41:08Z

go/vt/vttablet/tabletserver/query_executor.go

+			waiterCap := qre.tsv.config.ConsolidatorQueryWaiterCap
+			if waiterCap == 0 || c.WaiterCountOfTotal() <= waiterCap {
+				qre.logStats.QuerySources |= tabletenv.QuerySourceConsolidator
+				startTime := time.Now()
+				q.Wait()
+				qre.tsv.stats.WaitTimings.Record("Consolidations", startTime)
+			}


Adding the waiter in Record is too late. As the wait is already complete in q.Wait().
Post that q.Result() returns the stored result.

c.WaiterCountOfTotal() <= waiterCap - This check will allow concurrent threads to go through without respecting the limit.

Hi @harshit-gangal Could you please let me know if I understand correctly regarding your concern.

c.WaiterCountOfTotal() <= waiterCap - This check will allow concurrent threads to go through without respecting the limit.

Same query won't run concurrently, because q is a same *pendingResult object

Different queries when run concurrently (for those following same queries), this solution uses totalWaiterCount and only record those queries that use Wait() for *pendingResult. For example, if there are 2 different queries, and:
-- select a from A (1 running, 100 waiting)
-- select b from B (1 running, 100 waiting)
Then totalWaiterCount is 100+100=200, I added testCase on this, which is concurrently update totalWaiterCount and verify above theory is correct also verify no data race when on concurrent.

Uniq queries should only enter if original {...} path, so it does not run c.WaiterCountOfTotal() <= waiterCap check.

c.WaiterCountOfTotal() <= waiterCap - This check will allow concurrent threads to go through without respecting the limit.

WaiterCountOfTotal() is a concurrent safe, so it should be respected the limit.

func (co *consolidator) WaiterCountOfTotal() int64 { return atomic.LoadInt64(co.totalWaiterCount) }

Adding the waiter in Record is too late. As the wait is already complete in q.Wait().
Post that q.Result() returns the stored result.

Record is already used to increase the count of query, for queries passed c.WaiterCountOfTotal() <= waiterCap check, seems a good space to increase totalWaiterCount as well.

harshit-gangal · 2024-12-13T08:03:54Z

changelog/22.0/22.0.0/summary.md

@@ -7,6 +7,9 @@
  - **[Prefer not promoting a replica that is currently taking a backup](#reparents-prefer-not-backing-up)**
  - **[VTOrc Config File Changes](#vtorc-config-file-changes)**

+- **[Minor Changes](#minor-changes)**
+  - **[VTTablet](#vttablet)**
+  - [VTTablet: New ResetSequences RPC](#vttablet-consolidator-query-waiter-cap)


VTTablet: New ResetSequences RPC -> Query Consolidation Waiter Cap

harshit-gangal · 2024-12-13T08:57:15Z

go/vt/vttablet/tabletserver/query_executor.go

+			if waiterCap == 0 || c.WaiterCountOfTotal() <= waiterCap {
+				qre.logStats.QuerySources |= tabletenv.QuerySourceConsolidator
+				startTime := time.Now()
+				q.Wait()


The waiter count increases on calling the q.Wait() method.
If you run 10s or 100s of request, they can still pass the check c.WaiterCountOfTotal() <= waiterCap before any new waiter not getting consolidated.
A concurrent test will help test if the waiter count exceeds the maximum allowed waiter.

I think the better place to update the wait counter will be in create call

Signed-off-by: Jun Wang <[email protected]>

jwangace requested review from harshit-gangal, systay, shlomi-noach, rohit-nayak-ps, timvaillancourt and deepthi as code owners November 16, 2024 16:29

github-actions bot added this to the v22.0.0 milestone Nov 16, 2024

jwangace force-pushed the issues_17243 branch from eac658f to b08fc4d Compare November 16, 2024 16:48

harshit-gangal reviewed Nov 20, 2024

View reviewed changes

go/vt/vttablet/tabletserver/tabletenv/config.go Outdated Show resolved Hide resolved

harshit-gangal added the release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) label Nov 20, 2024

harshit-gangal reviewed Nov 20, 2024

View reviewed changes

jwangace force-pushed the issues_17243 branch from 2e429b6 to 86df77e Compare November 21, 2024 00:58

jwangace force-pushed the issues_17243 branch 4 times, most recently from 48f184e to 7cb8366 Compare November 21, 2024 03:32

jwangace force-pushed the issues_17243 branch 8 times, most recently from 6739ea6 to 8a5715e Compare November 29, 2024 21:36

jwangace requested a review from harshit-gangal November 30, 2024 00:32

jwangace changed the title ~~--consolidator-query-waiter-cap to set the max number of clients waiting for consolidated query~~ --consolidator-query-waiter-cap to set the max number of waiter for consolidated query Nov 30, 2024

jwangace force-pushed the issues_17243 branch 2 times, most recently from 3317e4a to 2d8cf24 Compare December 2, 2024 17:01

harshit-gangal reviewed Dec 3, 2024

View reviewed changes

jwangace force-pushed the issues_17243 branch 8 times, most recently from c526b3a to 8e62c33 Compare December 4, 2024 07:41

jwangace requested a review from harshit-gangal December 6, 2024 16:58

harshit-gangal reviewed Dec 13, 2024

View reviewed changes

jwangace force-pushed the issues_17243 branch 3 times, most recently from 404ac98 to 2926cb0 Compare December 15, 2024 02:32

update solution

e5cdf1a

Signed-off-by: Jun Wang <[email protected]>

jwangace force-pushed the issues_17243 branch from 2926cb0 to e5cdf1a Compare December 15, 2024 02:40

jwangace requested a review from harshit-gangal December 15, 2024 03:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--consolidator-query-waiter-cap to set the max number of waiter for consolidated query #17244

--consolidator-query-waiter-cap to set the max number of waiter for consolidated query #17244

jwangace commented Nov 16, 2024 •

edited

Loading

vitess-bot bot commented Nov 16, 2024

harshit-gangal left a comment

harshit-gangal commented Nov 20, 2024

jwangace commented Nov 21, 2024

jwangace commented Nov 21, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024 •

edited

Loading

jwangace commented Nov 30, 2024

harshit-gangal Dec 3, 2024

jwangace Dec 4, 2024

harshit-gangal Dec 3, 2024

jwangace Dec 4, 2024 •

edited

Loading

harshit-gangal Dec 13, 2024

harshit-gangal Dec 13, 2024

--consolidator-query-waiter-cap to set the max number of waiter for consolidated query #17244

Are you sure you want to change the base?

--consolidator-query-waiter-cap to set the max number of waiter for consolidated query #17244

Conversation

jwangace commented Nov 16, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Nov 16, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

harshit-gangal left a comment

Choose a reason for hiding this comment

harshit-gangal commented Nov 20, 2024

jwangace commented Nov 21, 2024

jwangace commented Nov 21, 2024 • edited Loading

codecov bot commented Nov 21, 2024 • edited Loading

Codecov Report

jwangace commented Nov 30, 2024

harshit-gangal Dec 3, 2024

Choose a reason for hiding this comment

jwangace Dec 4, 2024

Choose a reason for hiding this comment

harshit-gangal Dec 3, 2024

Choose a reason for hiding this comment

jwangace Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

harshit-gangal Dec 13, 2024

Choose a reason for hiding this comment

harshit-gangal Dec 13, 2024

Choose a reason for hiding this comment

jwangace commented Nov 16, 2024 •

edited

Loading

jwangace commented Nov 21, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024 •

edited

Loading

jwangace Dec 4, 2024 •

edited

Loading