Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VReplication: Estimate lag when workflow fully throttled (#16577) #147

Closed

Conversation

mhamza15
Copy link

@mhamza15 mhamza15 commented Mar 24, 2025

@mhamza15 mhamza15 self-assigned this Mar 24, 2025
@Copilot Copilot bot review requested due to automatic review settings March 24, 2025 18:28
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR backports changes from an upstream PR to improve how replication lag is estimated when a workflow is fully throttled. Key changes include:

  • Updating the throttler app name used in tests.
  • Adding logic to confirm and validate throttling metrics in tests.
  • Refining lag estimation in the vplayer and cleaning up heartbeat injection in the vstreamer.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
go/test/endtoend/vreplication/vreplication_test.go Updated app name in throttler usage and added validation via confirmVReplicationThrottling.
go/vt/vttablet/tabletmanager/vreplication/vplayer.go Improved lag estimation logic and updated throttling calculations in applyEvents.
go/vt/vttablet/tabletserver/vstreamer/vstreamer.go Removed an unused rate limiter and slightly restructured heartbeat injection logic.
Comments suppressed due to low confidence (3)

go/test/endtoend/vreplication/vreplication_test.go:61

  • Ensure that changing targetThrottlerAppName from VReplicationName to VPlayerName is intentional and consistent with how throttling is expected to operate in these tests.
targetThrottlerAppName = throttlerapp.VPlayerName

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

  • Review the logic that excludes throttled heartbeat events from lag computation to ensure that it correctly handles lag estimation when batches include mixed event types.
if !(event.Type == binlogdatapb.VEventType_HEARTBEAT && event.Throttled) {

go/vt/vttablet/tabletserver/vstreamer/vstreamer.go:309

  • Confirm that removing the rate limiter for throttled heartbeats does not lead to excessive heartbeat injections during throttling periods.
throttledHeartbeatsRateLimiter.Do(func() error { return injectHeartbeat(true) })

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

@mhamza15
Copy link
Author

Going to test on vitesst. Control case:

  1. Running a MoveTables from vitesst-unsharded to vitesst-sharded
  2. Generate writes using sysbench
  3. Note throttler metrics healthy and 0 replication lag
  4. Stop/disable vttablet on a replica
  5. Note throttler metrics unhealthy but still 0 replication lag

Test case:

  1. Running a MoveTables from vitesst-unsharded to vitesst-sharded
  2. Generate writes using sysbench
  3. Note throttler metrics healthy and 0 replication lag
  4. Stop/disable vttablet on a replica
  5. Note throttler metrics unhealthy and non-zero estimated replication lag

@mhamza15
Copy link
Author

mhamza15 commented Apr 1, 2025

Closed in favor of #148

@mhamza15 mhamza15 closed this Apr 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants