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: make flags workflow-specific and dynamically changeable #16583

Merged
merged 58 commits into from
Sep 18, 2024

Conversation

rohit-nayak-ps
Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps commented Aug 12, 2024

Description

Currently several configuration options for VReplication Workflows are vttablet flags. This means that any change requires restarts of vttablets. This PR allows Workflow Update to set these flags for a specific workflow. On a workflow init the config overrides are merged into the default values (which are the ones specified via the vttablet flags).

A major refactor was required to pass the config through to the various components where the vttablet flag values were directly used.

The workflow's config is also passed along to the source VStreamers (for both the copy and binlog player phases). This allows the user to override the flags on the source vttablet's as well by just specifying it for the workflow. If not overridden, the flag values are picked up from the source and not the target for the vstreamers.

A stats object (accessible via debug/vars) is exported per workflow stream with the current merged config values for that stream.

Note:

  • Config overrides can currently done while creating MoveTables or Reshard workflows or for any running workflow using Workflow Update.
  • Any change to the configuration should be made via WorkflowUpdate, otherwise the existing controllers will not pick up the new values.
  • Any change to the vttablet flags will only apply to a workflow if they are not overriden. If overriden, you need to use Workflow Update.

Persistence

A new sub-object config has been added to the options json document column in _vt.vreplication. This is a map of key/value pairs, for the overriden flags, with their workflow-specific values. The key is the flag name and value a string representation of the value. Appropriate serialization and deserialization is done while storing the values and reading it back.

Related Changes

  • When ever a workflow starts or resumes, the vreplication controller on the target loads the config values into its local scope
  • All code locations which accessed the flag values have been modified to use the locally scoped values
  • Tests and API signature changes required a lot of related tedious code change

List of all current dynamic flags

S. No. Flag Name Current Default Value
1 vreplication_max_time_to_retry_on_error 0s
2 vstream_dynamic_packet_size true
3 vreplication_net_write_timeout 600
4 vreplication_heartbeat_update_interval 1
5 vreplication-parallel-insert-workers 1
6 vstream_packet_size 250000
7 vreplication_experimental_flags 3
8 vreplication_retry_delay 5s
9 relay_log_max_size 250000
10 vstream_binlog_rotation_threshold 67108864
11 vreplication_store_compressed_gtid false
12 vreplication_net_read_timeout 300
13 vreplication_copy_phase_duration 1h
14 relay_log_max_items 5000
15 vreplication_replica_lag_tolerance 1m

Related Issue(s)

Fixes #16489

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

@github-actions github-actions bot added this to the v21.0.0 milestone Aug 12, 2024
@rohit-nayak-ps rohit-nayak-ps added Component: VReplication Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Aug 12, 2024
Copy link

codecov bot commented Aug 15, 2024

Codecov Report

Attention: Patch coverage is 78.71094% with 109 lines in your changes missing coverage. Please review.

Project coverage is 69.52%. Comparing base (d054447) to head (1dcbaec).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
go/vt/vttablet/common/flags.go 33.33% 14 Missing ⚠️
.../vtctldclient/command/vreplication/common/utils.go 7.14% 13 Missing ⚠️
.../vttablet/tabletmanager/vreplication/controller.go 78.04% 9 Missing ⚠️
...dclient/command/vreplication/materialize/create.go 0.00% 6 Missing ⚠️
...tctldclient/command/vreplication/reshard/create.go 0.00% 6 Missing ⚠️
...ctldclient/command/vreplication/workflow/update.go 0.00% 6 Missing ⚠️
go/vt/binlog/binlogplayer/mock_dbclient.go 50.00% 6 Missing ⚠️
go/vt/vttablet/tabletmanager/vreplication/stats.go 14.28% 6 Missing ⚠️
...ablet/tabletmanager/vreplication/vcopier_atomic.go 0.00% 5 Missing ⚠️
...ldclient/command/vreplication/movetables/create.go 0.00% 4 Missing ⚠️
... and 18 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16583      +/-   ##
==========================================
+ Coverage   68.93%   69.52%   +0.58%     
==========================================
  Files        1566     1567       +1     
  Lines      202047   202388     +341     
==========================================
+ Hits       139286   140709    +1423     
+ Misses      62761    61679    -1082     

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

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.

Nice work on this! I pushed a commit to address my own nits. If any tests fail then I'll push another commit to fix them.

go/vt/vttablet/tabletserver/vstreamer/vstreamer.go Outdated Show resolved Hide resolved
go/vt/vttablet/common/config.go Show resolved Hide resolved
go/vt/vttablet/common/config.go Show resolved Hide resolved
@deepthi deepthi added the release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) label Sep 12, 2024
@@ -2362,6 +2361,7 @@ func (s *Server) WorkflowUpdate(ctx context.Context, req *vtctldatapb.WorkflowUp
span.Annotate("tablet_types", req.TabletRequest.TabletTypes)
span.Annotate("on_ddl", req.TabletRequest.OnDdl)
span.Annotate("state", req.TabletRequest.State)
span.Annotate("config_overrides", req.TabletRequest.ConfigOverrides)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a standard naming convention for these annotations?

go/vt/vttablet/common/flags.go Show resolved Hide resolved
go/vt/vttablet/common/flags.go Outdated Show resolved Hide resolved
go/vt/vttablet/tabletmanager/framework_test.go Outdated Show resolved Hide resolved
@rohit-nayak-ps rohit-nayak-ps force-pushed the rohit/vr-dynamic-flags2 branch 2 times, most recently from bc5a2e9 to 111541c Compare September 18, 2024 10:48
@rohit-nayak-ps rohit-nayak-ps removed the release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) label Sep 18, 2024
… 'Workflow update' and display with 'Workflow get'. No validations on keys or values atm

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
… common default and init flag defaults from it.

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
rohit-nayak-ps and others added 24 commits September 18, 2024 19:22
…rror early. Related refactor. Add debug/env VReplicationConfig stats to expose currently used flags

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
…kdb logs. Properly init workflow config in vreplication unit tests

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
… and not locally

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps rohit-nayak-ps merged commit 56c39b2 into vitessio:main Sep 18, 2024
100 checks passed
@rohit-nayak-ps rohit-nayak-ps deleted the rohit/vr-dynamic-flags2 branch September 18, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: VReplication Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VReplication: allow overriding flags per-workflow and dynamically
3 participants