VReplication: Estimate lag when workflow fully throttled #16577

mattlord · 2024-08-10T17:37:12Z

Description

We did not properly calculate the vreplication lag for a workflow when it was fully throttled — on the source/vstreamer side or the target/vplayer side.

This PR ensures that we account for these scenarios and estimate the vreplication lag for the workflow when they occur.

Related Issue(s)

Fixes: Bug Report: VReplication lag is not updated when vplayer is throttled #16575

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

codecov · 2024-08-10T17:59:43Z

Codecov Report

Attention: Patch coverage is 86.95652% with 3 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@f2d5d1c). Learn more about missing BASE report.
Report is 6 commits behind head on main.

Files	Patch %	Lines
go/vt/vttablet/tabletserver/vstreamer/vstreamer.go	71.42%	2 Missing ⚠️
.../vt/vttablet/tabletmanager/vreplication/vplayer.go	93.75%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #16577   +/-   ##
=======================================
  Coverage        ?   68.79%           
=======================================
  Files           ?     1557           
  Lines           ?   199895           
  Branches        ?        0           
=======================================
  Hits            ?   137515           
  Misses          ?    62380           
  Partials        ?        0

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

shlomi-noach

While this PR is still in Draft, it looks good to me. Thank you!

go/vt/vtctl/workflow/materializer.go

Signed-off-by: Matt Lord <[email protected]>

This did not have a practical impact but was obviously wrong Signed-off-by: Matt Lord <[email protected]>

Revert "Fix vitessio#16565" Signed-off-by: Matt Lord <[email protected]>

Signed-off-by: Matt Lord <[email protected]>

mattlord · 2024-08-13T05:22:46Z

go/vt/vttablet/tabletserver/vstreamer/vstreamer.go

-				throttledHeartbeatsRateLimiter.Do(func() error {
-					return injectHeartbeat(true, checkResult.Summary())
-				})


I realized that we were getting double the heartbeats as we injected them here and via the timer. I consolidated them into the main loop with the timer. Otherwise we needed to do twice the work as the other heartbeats would get interspersed with these and those would indicate that we weren't throttled when in fact we were — so we needed to check the throttler there as well and reset the heartbeat timer in injectHeartbeat() to limit the extra heartbeats and even then some extras still went through as it was a race.

shlomi-noach

I'm not sure I understand some of the changes in the last force-push, asking for clarifications inline.

go/vt/vttablet/tabletmanager/vreplication/vplayer.go

shlomi-noach · 2024-08-13T06:09:21Z

go/vt/vttablet/tabletserver/vstreamer/vstreamer.go

-				})
-				// we won't process events, until we're no longer throttling
-				logger.Infof("throttled.")
+				logger.Infof("vstreamer throttled%s: %s.", wfNameLog, checkResult.Summary())


Would this spam the logs when throttled?

It's a throttled logger that logs the message at most once every 5 minutes. It was added in #14936. I thought about removing these altogether as the value was not clear to me, but instead I at least made this one much more useful IMO as it would state the component and the workflow.

shlomi-noach · 2024-08-13T06:10:08Z

go/vt/vttablet/tabletserver/vstreamer/vstreamer.go

@@ -394,7 +391,8 @@ func (vs *vstreamer) parseEvents(ctx context.Context, events <-chan mysql.Binlog
 		case <-ctx.Done():
 			return nil
 		case <-hbTimer.C:
-			if err := injectHeartbeat(false, ""); err != nil {
+			checkResult, ok := vs.vse.throttlerClient.ThrottleCheckOKOrWaitAppName(ctx, vs.throttlerApp)


Could you please explain what this throttler check is for?

It was added as part of consolidating the two heartbeat injectors: #16577 (comment)

We had two goroutines doing heartbeat injections. One that was sent only when we were throttled (removed here), and this one that would be sent both when we were throttled AND when we were not. This one would always say that we weren't throttled — even when in fact we were — so it prevented us from being able to estimate the lag in the vplayer when the vstreamer was fully throttled.

In that case I encourage you to use vs.vse.throttlerClient.ThrottleCheckOK as opposed to vs.vse.throttlerClient.ThrottleCheckOKOrWaitAppName. You're not interested in the extra sleep.

This makes sense now, thank you for clarifying.

Ah, I see. Thank you! 🙇 That makes total sense — no need for the sleep here as it's not done in a tight loop. I should have at least checked the function comments. 🙂 I've made this change.

Signed-off-by: Matt Lord <[email protected]>

rohit-nayak-ps

Nice catch and fix.

mattlord added Type: Bug Component: VReplication Backport to: release-20.0 Needs to be backport to release-20.0 labels Aug 10, 2024

github-actions bot added this to the v21.0.0 milestone Aug 10, 2024

mattlord added the Component: Observability Pull requests that touch tracing/metrics/monitoring label Aug 10, 2024

shlomi-noach approved these changes Aug 11, 2024

View reviewed changes

mattlord commented Aug 12, 2024

View reviewed changes

go/vt/vtctl/workflow/materializer.go Outdated Show resolved Hide resolved

deepthi reviewed Aug 12, 2024

View reviewed changes

go/vt/vtctl/workflow/materializer.go Outdated Show resolved Hide resolved

mattlord added 2 commits August 12, 2024 13:37

Estimate lag when vplayer throttled

e7f0c45

Signed-off-by: Matt Lord <[email protected]>

Fix vitessio#16565

f1826cc

This did not have a practical impact but was obviously wrong Signed-off-by: Matt Lord <[email protected]>

mattlord force-pushed the vplayer_throttled_lag branch from b1e1e85 to ee17b78 Compare August 12, 2024 17:37

mattlord requested a review from deepthi August 12, 2024 17:38

mattlord changed the title ~~VReplication: Estimate lag when vplayer throttled~~ VReplication: Estimate lag when workflow throttled Aug 13, 2024

mattlord changed the title ~~VReplication: Estimate lag when workflow throttled~~ VReplication: Estimate lag when workflow fully throttled Aug 13, 2024

mattlord removed the Backport to: release-20.0 Needs to be backport to release-20.0 label Aug 13, 2024

mattlord force-pushed the vplayer_throttled_lag branch from 812297c to 5ccf0d9 Compare August 13, 2024 02:56

mattlord added 6 commits August 12, 2024 22:59

Update e2e test

58605b3

Revert "Fix vitessio#16565" Signed-off-by: Matt Lord <[email protected]>

Fixup test changes/additions

a163ec9

Signed-off-by: Matt Lord <[email protected]>

Properly estimate lag when vstreamer is fully throttled

5914378

Signed-off-by: Matt Lord <[email protected]>

Improve table plan logs

9dd061a

Signed-off-by: Matt Lord <[email protected]>

Changes after self review and further testing

77c1d1a

Signed-off-by: Matt Lord <[email protected]>

Create a PR dedicated to improving the table plan logging

ee25994

Signed-off-by: Matt Lord <[email protected]>

mattlord force-pushed the vplayer_throttled_lag branch from 5ccf0d9 to ee25994 Compare August 13, 2024 02:59

mattlord requested a review from shlomi-noach August 13, 2024 03:20

mattlord marked this pull request as ready for review August 13, 2024 03:20

mattlord requested review from harshit-gangal, systay, rohit-nayak-ps and timvaillancourt as code owners August 13, 2024 03:20

mattlord removed request for systay, timvaillancourt and harshit-gangal August 13, 2024 03:20

mattlord added 2 commits August 13, 2024 00:10

Tweak behavior

001b4fb

Signed-off-by: Matt Lord <[email protected]>

Simplify

9d2a09f

Signed-off-by: Matt Lord <[email protected]>

mattlord commented Aug 13, 2024

View reviewed changes

shlomi-noach reviewed Aug 13, 2024

View reviewed changes

mattlord added 2 commits August 13, 2024 08:48

Improve var name

fb8741e

Signed-off-by: Matt Lord <[email protected]>

Check throttler w/o sleep as it's not done in a loop

919056a

Signed-off-by: Matt Lord <[email protected]>

rohit-nayak-ps approved these changes Aug 13, 2024

View reviewed changes

shlomi-noach approved these changes Aug 13, 2024

View reviewed changes

mattlord merged commit 95c77e9 into vitessio:main Aug 15, 2024
129 checks passed

mattlord deleted the vplayer_throttled_lag branch August 15, 2024 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VReplication: Estimate lag when workflow fully throttled #16577

VReplication: Estimate lag when workflow fully throttled #16577

mattlord commented Aug 10, 2024 •

edited

Loading

codecov bot commented Aug 10, 2024 •

edited

Loading

shlomi-noach left a comment

mattlord Aug 13, 2024 •

edited

Loading

shlomi-noach left a comment

shlomi-noach Aug 13, 2024

mattlord Aug 13, 2024 •

edited

Loading

shlomi-noach Aug 13, 2024

shlomi-noach Aug 13, 2024

mattlord Aug 13, 2024

shlomi-noach Aug 13, 2024

mattlord Aug 13, 2024

rohit-nayak-ps left a comment

VReplication: Estimate lag when workflow fully throttled #16577

VReplication: Estimate lag when workflow fully throttled #16577

Conversation

mattlord commented Aug 10, 2024 • edited Loading

Description

Related Issue(s)

Checklist

codecov bot commented Aug 10, 2024 • edited Loading

Codecov Report

shlomi-noach left a comment

Choose a reason for hiding this comment

mattlord Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

shlomi-noach left a comment

Choose a reason for hiding this comment

shlomi-noach Aug 13, 2024

Choose a reason for hiding this comment

mattlord Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

shlomi-noach Aug 13, 2024

Choose a reason for hiding this comment

shlomi-noach Aug 13, 2024

Choose a reason for hiding this comment

mattlord Aug 13, 2024

Choose a reason for hiding this comment

shlomi-noach Aug 13, 2024

Choose a reason for hiding this comment

mattlord Aug 13, 2024

Choose a reason for hiding this comment

rohit-nayak-ps left a comment

Choose a reason for hiding this comment

mattlord commented Aug 10, 2024 •

edited

Loading

codecov bot commented Aug 10, 2024 •

edited

Loading

mattlord Aug 13, 2024 •

edited

Loading

mattlord Aug 13, 2024 •

edited

Loading