--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#17244frouioui merged 2 commits intovitessio:mainfrom
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
eac658f to
b08fc4d
Compare
harshit-gangal
left a comment
There was a problem hiding this comment.
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
|
Website docs would also need an update https://vitess.io/docs/22.0/user-guides/configuration-advanced/query-consolidation/ |
2e429b6 to
86df77e
Compare
Updated. |
Hi @harshit-gangal This is not in this repo, is it? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17244 +/- ##
==========================================
+ Coverage 67.63% 67.65% +0.01%
==========================================
Files 1586 1586
Lines 255566 255576 +10
==========================================
+ Hits 172850 172907 +57
+ Misses 82716 82669 -47 ☔ View full report in Codecov by Sentry. |
48f184e to
7cb8366
Compare
6739ea6 to
8a5715e
Compare
Pushed new solution to use absolute count. |
3317e4a to
2d8cf24
Compare
There was a problem hiding this comment.
nit: this can be ignored to be kept in default config for 0 as default
There was a problem hiding this comment.
I dropped this line and use 0 as default in the flag.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 runc.WaiterCountOfTotal() <= waiterCapcheck.
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.
c526b3a to
8e62c33
Compare
changelog/22.0/22.0.0/summary.md
Outdated
There was a problem hiding this comment.
VTTablet: New ResetSequences RPC -> Query Consolidation Waiter Cap
Signed-off-by: Jun Wang <jun.wang@demonware.net>
| if waiterCap == 0 || *q.AddWaiterCounter(0) <= waiterCap { | ||
| qre.logStats.QuerySources |= tabletenv.QuerySourceConsolidator |
There was a problem hiding this comment.
I think this check is not thread safe, so multiple threads can pass this check and can exceed the waiter cap.
I believe this should be acceptable.
|
Hi @harshit-gangal Thank you so much for your approval! Could I kindly ask for your help to get a second review & approval from someone else? It would be valuable for us if Vitess could accept it, as it helps address the Pod OOM issues we’re facing. |
|
Hi @jwangace, can you merge |
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
Deployment Notes