TableGC: speed up GC process via RequestChecks(). Utilized by Online DDL for artifact cleanup
#14431
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
… CLEANUP process Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
shlomi-noach
added
Type: Enhancement
shlomi-noach
requested review from
harshit-gangal,
systay,
rohit-nayak-ps,
mattlord and
deepthi
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
NeedsDescriptionUpdate
|
|
shlomi-noach
removed
NeedsDescriptionUpdate
Signed-off-by: Shlomi Noach <[email protected]>
…d's validateAnyState() Signed-off-by: Shlomi Noach <[email protected]>
|
endtoend test fixed. |
dbussink
approved these changes
Nov 7, 2023
|
Request for additional review 🙏 |
mattlord
approved these changes
Nov 30, 2023
Comment on lines
363
to
377
| // ctx, cancel := context.WithTimeout(ctx, tableTransitionExpiration+gc.CheckTablesReentryMinInterval) | ||
| // defer cancel() | ||
|
|
||
| // ticker := time.NewTicker(time.Second) | ||
| // defer ticker.Stop() | ||
|
|
||
| // for { | ||
| // select { | ||
|
|
||
| // case <-ctx.Done(): | ||
| // case <-ticker.C: | ||
| // } | ||
| // } | ||
| // time.Sleep(tableTransitionExpiration + gc.CheckTablesReentryMinInterval) | ||
| // We're now both beyond table's timestamp as well as a tableGC interval |
There was a problem hiding this comment.
Do we need to keep this commented code around?
go/timer/suspendable_ticker_test.go
Outdated
| @@ -0,0 +1,140 @@ | |||
| /* | |||
| Copyright 2020 The Vitess Authors. | |||
go/timer/suspendable_ticker_test.go
Outdated
| func TestSuspendableTicker(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
| ticker := NewSuspendableTicker(10*time.Millisecond, false) |
There was a problem hiding this comment.
IMO it would be nicer to make this a variable and then we can use that as the base for later sleeps and asserts (*3 e.g.).
There was a problem hiding this comment.
The logic seems correct but I suspect we'll see some flakes in CI. Have we re-run this several times in a row to be sure it's at least mostly reliable?
| collector.removePurgingTable(tableName) | ||
| // Chances are, there's more tables waiting to be purged. Let's speed things by | ||
| // requesting another purge, instead of waiting a full purgeReentranceInterval cycle | ||
| purgeReentranceTicker.TickAfter(time.Second) |
There was a problem hiding this comment.
Should time.Second be a variable? I'm unsure why we're using that specific value here. I think it's a minimum we may use in various places over time, right?
There was a problem hiding this comment.
now a variable. And you're right, it's a minimum; first and foremost because it ensures the table name signature, which is partly composed of a timestamp, gets a unique value over time.
To be fair, the purge logic is going to eventually disappear. It's relevant to 5.7, and to old 8.0 versions. Eventually we'll retire this.
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
ejortegau
pushed a commit
to slackhq/vitess
that referenced
this pull request
Dec 13, 2023
…e DDL for artifact cleanup (vitessio#14431) Signed-off-by: Shlomi Noach <[email protected]>
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
Normally, the TableGC mechanism checks for GC table statuses once per hour. It will transition tables to their next state, potentially dealing with that new state yet only one hour later. There's incentive to speed up the process. But instead of reducing the interval (causing more load on the
PRIMARY), we choose to intelligently kick new check routines whenever there's a plausible reason -- or an explicit request.Any time a table changes state, calls for a new cycle that takes place within seconds. A public function,
RequestChecks(), makes it possible to "inform" the GC mechanism of recent changes that can benefit from GC.To combat load, the check function is non-reentrant, and discards recurring or flooding calls made within a period of 10 seconds (ie. the function only agrees to run once per 10 seconds at most).
This is in turn used by Online DDL's state machine: right after a
ALTER VITESS_MIGRATION ... CLEANUP(or similar command) is invoked, and assuming an actual artifact is affected, Online DDL calls GC'sRequestChecks()to speed up the destruction and cleanup of said artifact tables.Related Issue(s)
Fixes #14427
Checklist
Deployment Notes