Throttler: fix race conditions in Operate() termination and in tests #14971
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
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
shlomi-noach
requested review from
harshit-gangal,
systay and
rohit-nayak-ps
as code owners
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
|
vitess-bot
bot
added
NeedsBackportReason
shlomi-noach
removed
NeedsDescriptionUpdate
shlomi-noach
commented
Jan 17, 2024
| @@ -382,3 +383,51 @@ func TestProbesWhileOperating(t *testing.T) { | |||
| }) | |||
| }) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The following is mostly copy+paste from the now deleted throttler_exclude_race_test.go file, and with a few minor changes. Notably atomic.LoadInt64(...) and some cosmetics.
5 tasks
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14971 +/- ##
==========================================
+ Coverage 47.26% 47.28% +0.02%
==========================================
Files 1138 1138
Lines 238842 238846 +4
==========================================
+ Hits 112880 112937 +57
+ Misses 117368 117318 -50
+ Partials 8594 8591 -3 ☔ View full report in Codecov by Sentry. |
shlomi-noach
removed
the
NeedsIssue
dbussink
approved these changes
Jan 17, 2024
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
mattlord
approved these changes
Jan 17, 2024
Comment on lines
-460
to
-462
| throttler.aggregatedMetrics.Flush() | ||
| throttler.recentApps.Flush() | ||
| throttler.nonLowPriorityAppRequestsThrottled.Flush() |
There was a problem hiding this comment.
These are done in the reverse order now because they are in defers. I'm assuming that's fine.
There was a problem hiding this comment.
Yes, that's fine. Ordering is irrelevant here.
Comment on lines
660
to
663
| defer wg.Done() | ||
| defer throttler.aggregatedMetrics.Flush() | ||
| defer throttler.recentApps.Flush() | ||
| defer throttler.nonLowPriorityAppRequestsThrottled.Flush() |
There was a problem hiding this comment.
Any reason not to use a single deferred function, at least for the flushes? It's fine as-is, just curious.
There was a problem hiding this comment.
No reason at all. I can consolidate.
Comment on lines
392
to
393
| // While the throttler is disabled, it is technically safe to iterate those structures. However, `go test -race` disagrees, | ||
| // which is why this test is in this *exclude_race* file |
There was a problem hiding this comment.
This comment is now off, isn't it?
There was a problem hiding this comment.
Yes, I'll remove it.
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
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.
Description
This PR fixes the following race conditions in the tablet throttler:
Disable(), caches were flushed (cleared) before context as cancelled, and soOperate()'s goroutine could still re-populate them before actually terminating.Operate()'s goroutine to terminate. At the time we excluded those tests via//go:build !race, but:We introduce a
WaitConditionwith which the unit tests canWait()for the complete termination ofOperate()'s goroutine. We ensure to flush the caches at that very goroutine. We fix and improve the unit test, and we now include it in race tests.Related Issue(s)
#14967
Checklist
Deployment Notes