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: throttling info for both source and target; Online DDL propagates said info #10601

Merged
merged 30 commits into from
Jul 5, 2022

Conversation

shlomi-noach
Copy link
Contributor

@shlomi-noach shlomi-noach commented Jun 28, 2022

Description

This PR is primarily an enhancement to VReplication, adding visibility into throttling status of an active workflow.

In addition, Online DDL now utilizes said information and presents it as part of SHOW VITESS_MIGRATIONS.

Lastly, there's a deadlock solution to a particular throttling scenario.

Throttling visibility in VReplication

The following components in VReplication consult the throttler (way before this PR):

  • vstreamer, on source tablet
  • rowstreamer, on source tablet
  • vplayer, on target tablet
  • vcopier, on target tablet

As of this PR, all four further advertise their throttling state. The table _vt.vreplication now has two new columns:

  • time_throttled (unix time, seconds)
  • component_throttled (name of component)

These present the last known incident where a VReplication component was throttled. Example values might be:

             time_throttled: 1656420364
        component_throttled: vcopier

For vplayer and vcopier, whenever they are throttled, they now ask vreplicator to update those values, e.g. via:

vr.updateTimeThrottled("vplayer")

vreplicator rate-limits those writes, to at most one per second. It's fine if calls to updateTimeThrottled() are frequent. Some calls may be dropped. The database will only be affected at most once per second.

As for vstreamer and rowstreamer, they don't have immediate access to _vt.vreplication or to vreplicator, because they are on the source tablet. A new bool throttled proto field is added in both VEvent and VStreamRowsResponse. If vstreamer is throttled, it sends (rate limited) heartbeat events with Throttled: true. If VStreamRowsResponse is throttled, it sends (rate limited) responses where Throttled: true. vplayer and vcopier receive and identify those responses, respectively, and call updateTimeThrottled().

This means we can see in _vt.vreplication the last known throttling incident: when it happened, and which component was affected, one of vstreamer, rowstreamer, vplayer, vcopier.

This information does not include frequency of throttling, number of successful/rejected throttler checks, etc. We leave that to a future PR.

Online DDL throttling info

When reviewing running migrations, and for vitess migration, the online DDL executor now reads the above two columns, and propagate them as two new columns in _vt.schema_migrations table:

  • last_throttled_timestamp (TIMESTAMP)
  • component_throttled (textual)

These are only sampled once per minute, sometimes more frequently.

A deadlock scenario, now solved

With the introduction of on demand heartbeats, we've introduced a throttling deadlock scenario. Consider the following scenario (Copy of comment from this PR):

  • a vitess migration
  • with on demand heartbeats
  • the streamer running on a replica
  • the streamer was throttled for long enough
  • then vplayer and vcopier are blocked, waiting for the streamer to do something
  • since they are blocked, they're not running throttler checks
  • since streamer runs on replica, it only checks that replica
  • therefore no one asking for on-demand heartbeats
  • then, if the conditions for the streamer's throttling are done, the streamer then thinks there's replication lag, with nothing to remediate it.
  • it's a deadlock.

And so, once per reviewRunningMigrations(), and assuming there are running migrations, we ensure to hit a throttler check. This will kick on-demand heartbeats, unlocking the deadlock.

rowstreamer heartbeats

rowstreamer now sends heartbeats every 10sec. This means even if it's blocked on reading the next batch of rows (blocked, slow, hangs, for whatever reason), it still reports heartbeats. these are intercepted by vcopier which then updates _vt.vreplication.time_heartbeat.

Related Issue(s)

#6926
#10198

Checklist

  • "Backport me!" label has been added if this change should be backported
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

@shlomi-noach
Copy link
Contributor Author

I forgot to mention. One of the side effects to this PR, which is actually the root trigger to working on it:

Without this PR, a VReplication workflow that was throttled, would be identified by the Online DDL executor as "stale" after 10 minutes of inactivity, and terminated.

Now, with this PR, this does not happen because time_updated in _vt.vreplication is updated whenever a throttling is identified. Thus, Online DDL executor keeps seeing the migration as "fresh", and does not terminate it.

Signed-off-by: Shlomi Noach <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Jun 28, 2022

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
  • If a new flag is being introduced, review whether it is really needed. The flag names should be clear and intuitive (as far as possible), and the flag's help should be descriptive.
  • If a workflow is added or modified, each items in Jobs should be named in order to mark it as required. If the workflow should be required, the GitHub Admin should be notified.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should either include a link to an issue that describes the bug OR an actual description of the bug and how to reproduce, along with a description of the fix.

Non-trivial changes

  • There should be some code comments as to why things are implemented the way they are.

New/Existing features

  • Should be documented, either by modifying the existing documentation or creating new documentation.
  • New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • vtctl command output order should be stable and awk-able.

Signed-off-by: Shlomi Noach <[email protected]>
@shlomi-noach
Copy link
Contributor Author

Looking into unit test failures, which seem to be related to the RateLimiter.

@shlomi-noach
Copy link
Contributor Author

Obviously only failing in GitHub CI 😛 and not on local envs.

@shlomi-noach shlomi-noach requested a review from deepthi as a code owner June 29, 2022 07:27
@shlomi-noach
Copy link
Contributor Author

shlomi-noach commented Jun 29, 2022

Added the two new columns in ReplicationStatusResult, which then shows up in vtctl Workflow ... Show

@shlomi-noach
Copy link
Contributor Author

Also added:

rowstreamer now sends heartbeats every 10sec. This means even if it's blocked on reading the next batch of rows (blocked, slow, hangs, for whatever reason), it still reports heartbeats. these are intercepted by vcopier which then updates _vt.vreplication.time_heartbeat.

@shlomi-noach
Copy link
Contributor Author

Fixed all unit tests, new ones showing! Still working through them.

Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
@shlomi-noach
Copy link
Contributor Author

Phew, the tests were brutal but legit! Now good to go!

Copy link
Contributor

@mattlord mattlord left a comment

Choose a reason for hiding this comment

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

I like it! This will greatly improve the observability/debugability of VReplication workflows (especially when we enable tablet throttling by default). Thank you for working on it!

I had a few questions and comments, but nothing major. I'll look out for your responses and can approve quickly if needed.

@rohit-nayak-ps if you have time, would you mind giving it a pass too? Thanks!

go/test/endtoend/onlineddl/vrepl/onlineddl_vrepl_test.go Outdated Show resolved Hide resolved
go/test/endtoend/onlineddl/vrepl/onlineddl_vrepl_test.go Outdated Show resolved Hide resolved
go/vt/binlog/binlogplayer/binlog_player.go Outdated Show resolved Hide resolved
go/vt/binlog/binlogplayer/binlog_player.go Outdated Show resolved Hide resolved
go/vt/vttablet/onlineddl/executor.go Outdated Show resolved Hide resolved
@@ -98,7 +100,7 @@ const (
reverted_uuid,
is_view
) VALUES (
%a, %a, %a, %a, %a, %a, %a, %a, %a, FROM_UNIXTIME(NOW()), %a, %a, %a, %a, %a, %a, %a, %a
%a, %a, %a, %a, %a, %a, %a, %a, %a, NOW(), %a, %a, %a, %a, %a, %a, %a, %a
Copy link
Contributor

@mattlord mattlord Jul 1, 2022

Choose a reason for hiding this comment

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

OK, so here again it's a unix timestamp in seconds precision. I wonder why we can't use a timestamp field in the table -- then we have a contract for what the value represents and we know we'll get what we expect from from_unixtime() etc? That's a moot point though I think as we are already using bigint fields for timestamps in these tables and related code so uniformity takes precedence over stricter types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So just to clarify, the column type in this case is a TIMESTAMP and there was a long standing bug where we erroneously INSERTed a FROM_UNIXTIME() value, which of course did not make any sense.

We are now looking at a 1.5 year old decision, to use TIMESTAMP values in _vt.schema_migrations table, long before it was even associated with _vt.vreplication, and long before I knew of th eexistence of _vt.vreplication. This is how the code "grew" and now it's too late to change _vt.schema_migrations from TIMESTAMP and into INT.

The fact that we copy values from _vt.vreplication, that happen to be INT types, and into _vt.schema_migrations with TIMESTAMP types, is unfortunate, but also does not tell the entire story. For long, schema_migrations acted with gh-ost and pt-osc, oblivious of vreplication.

go/vt/vttablet/onlineddl/vrepl.go Outdated Show resolved Hide resolved
go/vt/vttablet/tabletmanager/vreplication/vcopier.go Outdated Show resolved Hide resolved
go/vt/vttablet/tabletmanager/vreplication/vreplicator.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps left a comment

Choose a reason for hiding this comment

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

lgtm

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]>
Signed-off-by: Shlomi Noach <[email protected]>
Copy link
Contributor

@mattlord mattlord left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you ❤️ The only thing left is documentation — you've proven yourself to be very studious in that regard though so I'm not concerned. Please let me know when the docs PR is ready and I'll review it. 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Query Serving Component: VReplication Type: Bug Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants