Table GC: rely on tm state to determine operation mode by shlomi-noach · Pull Request #11972 · vitessio/vitess

shlomi-noach · 2022-12-15T09:54:38Z

Description

This is a simplification and an improvement to the tablet's table garbage collector operation.

Currently, the collector periodically checks to see whether the tablet is a PRIMARY. If yes, it enables the general operation, enables tickers, probes tables etc. If not, it turns off the tickers and stops doing any GC work. But it keeps polling. There is a leaderCheckInterval = 5 * time.Second variable, and this is where and when the collector checks for the tablet type.

As of this PR, this logic is simplified, and the variable is removed. The garbage collector only runs on Primary tablets. As such, we just use the fact that the tablet's state manager Open()s and Close()s the collector when going into and out of Primary type, respectively. There is no more polling/interval. It happens when it needs to happen.

The function Operate() now gets called upon Open() and terminates upon Close() using a context. Some variables are removed from the class and made function-local.

Existing endtoend tests are left unchanged and validate that the new behavior is good.

Related Issue(s)

Feature Request: internal cleanup in Table GC, rely on tablet state to control operation #11986
Somewhat similar changes to Dynamic tablet throttler config: enable/disable, set metrics query/threshold #11604
Some discussion in vttestserver: make tablet_refresh_interval configurable and reduce default value #11918 (comment)

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Documentation was added or is not required

Deployment Notes

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vitess-bot · 2022-12-15T09:54:40Z

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

deepthi

LGTM!

deepthi · 2022-12-16T00:18:46Z

go/vt/vttablet/tabletserver/gc/tablegc.go

 	for {
+		if ctx.Err() != nil {
+			// cancelled
+			return


Hmm, should the error be returned?

good catch, fixed!

deepthi · 2022-12-16T00:33:18Z

I was debating whether this requires an issue. You have labeled it as an Enhancement, to me it looks like an Internal Cleanup without any user-facing change. As such, we can do without an issue, but we might want to change the label.

deepthi · 2022-12-16T00:55:54Z

The unit test failures are very weird. This PR doesn't do anything that can affect vtexplain at all.
If at all I would have expected these failures on #11941 but they seemed to have passed there.

EDIT: unit tests are passing locally with a clean checkout. You may need to merge main again, I'm chalking this up to a bad merge.

Question that needs to be resolved.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2022-12-18T06:12:00Z

Fixes #11986

shlomi-noach · 2022-12-18T06:12:39Z

@deepthi I both changed the label and created an issue, #11986

shlomi-noach · 2022-12-18T09:34:20Z

Unit test failures still taking place, even after merging latest main

shlomi-noach · 2022-12-19T17:29:23Z

Unit tests continue to fail for me on a clean checkout on a different box.

deepthi · 2022-12-19T20:00:26Z

Unit tests continue to fail for me on a clean checkout on a different box.

can you create an issue with details and tag @vitessio/query-serving?
It's quite mysterious because they also seem to be passing on most merges into main.

shlomi-noach · 2022-12-20T05:38:21Z

Created issue at #11995

go/vt/vttablet/tabletserver/gc/tablegc.go

…ests can turn it off Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rsajwani · 2022-12-21T21:57:25Z

go/vt/vttablet/tabletserver/gc/tablegc.go


-		isPrimary: (atomic.LoadInt64(&collector.isPrimary) > 0),
-		IsOpen:    (atomic.LoadInt64(&collector.isOpen) > 0),
+		IsOpen: (atomic.LoadInt64(&collector.isOpen) > 0),


Why LoadInt64 here whereas we have removed atomic from other places where we are reading isOpen ? do we not need synchronization anywhere else ?

That's because isOpen is only changed in Open() and Close(), which are both using stateMutex.Lock() to exclude each other. So those two functions don't need to LoadInt64. But Status() is not protected with that same mutex. The tradeoff is that it must read isOpen with LoadInt64 because it might run at the same time Open() or Close() are running.
An alternative is to add stateMutex.Lock() to Status(), but I prefer not to.

rsajwani · 2022-12-21T21:59:09Z

go/vt/vttablet/tabletserver/gc/tablegc.go


+	ctx := context.Background()
+	ctx, collector.cancelOperation = context.WithCancel(ctx)
+	go collector.operate(ctx)


moving operate from init to Open. Does this means GC will start later then what it is currently now. Do you see any issue because of that delay?

I don't see an issue, because Operate() works on intervals anyhow. There is no guarantee when exactly the cycles begin. The default cycle interval is 1h. So the precise timing of when the cycle begins is insignificant.

GuptaManan100 · 2022-12-22T05:12:55Z

go/vt/vttablet/tabletserver/gc/tablegc.go

 		case <-tableCheckTicker.C:
 			{
-				_ = collector.checkTables(ctx)
+				log.Info("TableGC: tableCheckTicker")


Won't this log every 5 seconds on a primary tablet?

Same comments for the other logs on tickers

Won't this log every 5 seconds on a primary tablet?

The default interval is 1h, and I see no reason why in production it would be any less then several minutes. In testing, where we set it to 5s - yes. But then, do we care?

I removed the log entry for log.Info("TableGC: purgeReentranceTicker"), which runs once per 1m. Still very spacious IMO and shouldn't be a problem logging once per minute, but now removed. The rest of logs are either once per hour, or upon something actually being done.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2023-01-02T14:16:23Z

I've made the necessary changes, just waiting to verify that all questions have been satisfied.

shlomi-noach · 2023-01-09T06:07:22Z

all righty, merging!

Table GC: rely on tm state to determine operation mode

5fbdab2

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: TabletManager labels Dec 15, 2022

shlomi-noach requested a review from a team December 15, 2022 09:54

shlomi-noach requested review from harshit-gangal, rohit-nayak-ps and systay as code owners December 15, 2022 09:54

shlomi-noach mentioned this pull request Dec 15, 2022

vttestserver: make tablet_refresh_interval configurable and reduce default value #11918

Merged

3 tasks

Merge branch 'main' into tablegc-operate-state

e48e688

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

deepthi previously approved these changes Dec 16, 2022

View reviewed changes

deepthi reviewed Dec 16, 2022

View reviewed changes

shlomi-noach added 2 commits December 18, 2022 07:59

Merge branch 'main' into tablegc-operate-state

774f643

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

return error

ce94989

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added Type: Internal Cleanup and removed Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Dec 18, 2022

shlomi-noach mentioned this pull request Dec 20, 2022

Bug Report: Unexplainable vtexplain CI failures in PR #11995

Closed

GuptaManan100 reviewed Dec 20, 2022

View reviewed changes

go/vt/vttablet/tabletserver/gc/tablegc.go Show resolved Hide resolved

added EnableTableGC (defaults 'true') in tablet env config, so that t…

398d022

…ests can turn it off Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

deepthi approved these changes Dec 21, 2022

View reviewed changes

rsajwani approved these changes Dec 21, 2022

View reviewed changes

GuptaManan100 approved these changes Dec 22, 2022

View reviewed changes

shlomi-noach added 2 commits December 26, 2022 09:12

remove log entry because it will run every minute

24a78f9

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Merge branch 'main' into tablegc-operate-state

f8a0308

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach merged commit d8f1788 into vitessio:main Jan 9, 2023

shlomi-noach deleted the tablegc-operate-state branch January 9, 2023 06:07

shlomi-noach mentioned this pull request Feb 21, 2023

Feature Request: internal cleanup in Table GC, rely on tablet state to control operation #11986

Closed

shlomi-noach mentioned this pull request Mar 14, 2023

Table GC: remove spammy log entry #12625

Merged

4 tasks

Conversation

shlomi-noach commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Checklist

Deployment Notes

Uh oh!

vitess-bot bot commented Dec 15, 2022 • edited by deepthi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

Uh oh!

deepthi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepthi commented Dec 16, 2022

Uh oh!

deepthi commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shlomi-noach commented Dec 18, 2022

Uh oh!

shlomi-noach commented Dec 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shlomi-noach commented Dec 18, 2022

Uh oh!

shlomi-noach commented Dec 19, 2022

Uh oh!

deepthi commented Dec 19, 2022

Uh oh!

shlomi-noach commented Dec 20, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shlomi-noach commented Jan 2, 2023

Uh oh!

shlomi-noach commented Jan 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shlomi-noach commented Dec 15, 2022 •

edited

Loading

vitess-bot bot commented Dec 15, 2022 •

edited by deepthi

Loading

deepthi commented Dec 16, 2022 •

edited

Loading

shlomi-noach commented Dec 18, 2022 •

edited

Loading