Pass Semi sync information from durability policy and use it to log discrepancies #9533

GuptaManan100 · 2022-01-19T12:08:50Z

Description

In release 13, we will introduce a new flag durability in vtctld and vtctl. That durability policy should be the one that reflects the semi-sync settings and we will eventually deprecate enable_semi_sync flag on the vttablets. But all of this cannot be accomplished in a single release because of upgrade downgrade considerations. So the plan is as follows -

Introduce the new flag durability
Pass the information all through the components and just document the discrepancies in configuration.
Keep using enable_semi_sync flag in v13, so the new flag is only used for logging but it is expected that vtctl, vtctld binaries of v13 will have the flag set.
In release 14, we would have the semi-sync information being passed everywhere, so we can deprecate enable_semi_sync and use the information coming from the durability policies.

This PR accomplishes this change of passing the information down and logging the differences for release 13.

Related Issue(s)

RFC - Durability and consensus in Vtorc #8975

Checklist

Should this PR be backported?
Tests were added or are not required
Documentation was added or is not required - Add docs for the durability policies website#955, Add multiple changes in release docs #9570

Deployment Notes

…er calling a reparent operation Signed-off-by: Manan Gupta <manan@planetscale.com>

…ameter Signed-off-by: Manan Gupta <manan@planetscale.com>

…rences with enable_semi_sync flag Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

…f enabled Signed-off-by: Manan Gupta <manan@planetscale.com>

…parameter Signed-off-by: Manan Gupta <manan@planetscale.com>

…arameter Signed-off-by: Manan Gupta <manan@planetscale.com>

… a parameter Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

…y-policy Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

…y-policy Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

…he changed type and not with the original Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 · 2022-01-22T18:17:56Z

Apart from the checks in this PR, I also made a change wherein we start using the durability policy information in fixSemiSync in #9552. That PR is 2 commits ahead of this PR -

1380e76 wherein we change fixSemiSyncReplication
39e6f09 which fixes some of the unit tests to set the correct durability policies and skips the backup tests.

On that PR, it can be observed that the only failing tests are from cluster 11, 16, 19, 20, 21, 26, docker_test_cluster_10 which all run Backup RPC and Upgrade Downgrade test for reparenting with old vtctl. Their are 2 failing tests, one TestSemiSyncSetupCorrectly and TestChangeTypeSemiSync which both test semi-sync settings. We expect these tests to fail since old vtclt does not have durability policy information and the new vttablets in that branch only fixSemiSync using the parameter passed down. The failure in tests concerned with Backup RPC is also expected, since we will no longer be fixing semiSync after a Backup operation succeeds. We will rely on vtorc to fix it. The main reason for this -

After the durability policy changes take effect, a tablet might be sending semi sync acks to one primary but may be configured to not send semi sync acks to some other primary, and since Backup is a long operation during which the primary might have changed, we will have no idea whether we should be setting our semi-sync or not. So we will rely on vtorc to fix it when it becomes a compulsory component of Vitess. An alternate is to run StartReplication RPC after running Backup RPC, and that will figure out the semi sync settings.

shlomi-noach

I don't have much quality review content for now, just a few naming suggestions.

go/vt/orchestrator/inst/tablet_dao.go

go/vt/vttablet/tabletmanager/rpc_actions.go

go/cmd/vtworker/vtworker.go

go/vt/vttablet/tabletmanager/rpc_replication.go

go/vt/worker/multi_split_diff.go

deepthi · 2022-01-25T02:14:02Z

web/vtadmin/src/proto/vtadmin.d.ts

+
+        /** VEvent keyspace */
+        keyspace?: (string|null);
+
+        /** VEvent shard */
+        shard?: (string|null);


@doeg @ajm188 these fields were added to the proto definition some time ago, but the vtadmin files don't seem to have been regenerated at that time.

Can one of you post reminders in #maintainers and #developers so that people don't miss the step of generating vtadmin files?

I've posted another reminder to both channels. (This was also mentioned in #developers back at the beginning of December.) I expect this will (understandably) be an issue until we can enforce this in CI. The ticket for that is #9571.

deepthi · 2022-01-25T02:14:54Z

web/vtadmin/src/proto/vtadmin.js

+        /**
+         * ApplySchemaRequest sql_mode.
+         * @member {string} sql_mode
+         * @memberof tabletmanagerdata.ApplySchemaRequest
+         * @instance
+         */
+        ApplySchemaRequest.prototype.sql_mode = "";
+
        /**


This seems to be an artifact of a different PR as well.

deepthi · 2022-01-25T02:18:31Z

web/vtadmin/src/proto/vtadmin.js

@@ -35887,6 +35932,7 @@ $root.tabletmanagerdata = (function() {
         * Properties of a StartReplicationRequest.
         * @memberof tabletmanagerdata
         * @interface IStartReplicationRequest
+         * @property {boolean|null} [semiSync] StartReplicationRequest semiSync


@notfelineit do these changes affect the "destructive" tablet actions UI?

I don't think so, not as it exists - should we add a semiSync/durability option in the UI?

I think we might have to. Do you have a screenshot handy of what this looks like on the UI today?

Yes! It follows vtctld2's ui in that there is a singular button

I suspect that this directly calls the TabletManager grpc API instead of going through vtctld API.
My suggestion is to add a new vtctld API. Then semi_sync won't need to be provided by the user, it can be computed by VtctldServer.

I'll check today and see which it is using - and switch to vtctld if necesary.

@deepthi vtadmin uses this codepath: https://cs.github.com/vitessio/vitess/blob/f906693c6441f4b41d8f842ffb9dba85258b5158/go/vt/vtctl/grpcvtctldserver/server.go?q=StartReplication#L2439 it does call the tablet manager API, but I think this is the vtctld API mentioned? it calls IsReplicaSemiSync to provide semi-sync setting.

deepthi

Nice work.
Please address all feedback and then this can be merged.

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 added 14 commits January 13, 2022 15:17

test: add an e2e test verifying that semi sync is setup correctly aft…

ae9deed

…er calling a reparent operation Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: change PromoteReplica rpc to allow passing in semiSync as a par…

7cab69e

…ameter Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: add semiSync as a parameter to fixSemiSync and use it log diffe…

f18174b

…rences with enable_semi_sync flag Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: change parameter in fixSemiSync to be an enum

5413fd5

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: change InitReplica rpc to allow passing in semiSync as a parameter

d93d77f

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: change InitPrimary rpc to allow passing in semiSync as a parameter

0ba29a5

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: fix demotePrimary function to fix primary side semi-sync only i…

f87b857

…f enabled Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: change UndoDemotePrimary rpc to allow passing in semiSync as a …

1377080

…parameter Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: change UndoDemoteMaster rpc to allow passing in semiSync as a p…

8f045dd

…arameter Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: change SetReplicationSource rpc to allow passing in semiSync as…

1de77a3

… a parameter Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: change SetMaster rpc to allow passing in semiSync as a parameter

036dbb5

Signed-off-by: Manan Gupta <manan@planetscale.com>

Merge remote-tracking branch 'upstream/main' into semi-sync-durabilit…

7a83481

…y-policy Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: fix startReplication to not change semi-sync settings

bad3a0c

Signed-off-by: Manan Gupta <manan@planetscale.com>

test: pass correct durability flag to vtctld in reparent tests

1f83942

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 requested review from ajm188, deepthi, doeg and shlomi-noach as code owners January 19, 2022 12:08

GuptaManan100 added the Do Not Merge label Jan 19, 2022

GuptaManan100 marked this pull request as draft January 19, 2022 13:10

GuptaManan100 added 6 commits January 22, 2022 15:12

feat: fix changeTypeLocked to allow passing semiSync as a parameter

f6ea84a

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: add durability flag to vtworker

82e5a47

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: fix StartReplication to allow passing semiSync as a parameter

8ac91e1

Signed-off-by: Manan Gupta <manan@planetscale.com>

Merge remote-tracking branch 'upstream/main' into semi-sync-durabilit…

2fe8592

…y-policy Signed-off-by: Manan Gupta <manan@planetscale.com>

test: fix TestChangeTabletType to have the primary tablet setup in topo

a696c98

Signed-off-by: Manan Gupta <manan@planetscale.com>

test: fix TestStartReplication to have the primary tablet setup in topo

c5e6136

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 added Component: Cluster management Component: VTorc Vitess Orchestrator integration release notes and removed Do Not Merge labels Jan 22, 2022

GuptaManan100 added the Type: Internal Cleanup label Jan 22, 2022

GuptaManan100 added 8 commits January 22, 2022 17:18

test: add SettingDurabilityPolicies in init function for vtworker tests

db477a1

Signed-off-by: Manan Gupta <manan@planetscale.com>

test: SetDurabilityPolicies in endtoend tests for InitShardPrimary

243bfc3

Signed-off-by: Manan Gupta <manan@planetscale.com>

test: add semi-sync durability flag to all the e2e tests using semi-sync

9fb94ec

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: In ChangeTabletType we should query the durability rules with t…

8a75a7b

…he changed type and not with the original Signed-off-by: Manan Gupta <manan@planetscale.com>

test: only add the durability flag to vtctl if major version exceeds 13

128a52c

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: fix demotePrimary to preserve behaviour

ffbef3f

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: do not log error messages when SemiSyncActionNone

b19bdda

Signed-off-by: Manan Gupta <manan@planetscale.com>

test: add test for checking logs in fixSemiSync

ddcaa78

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 marked this pull request as ready for review January 22, 2022 18:07

GuptaManan100 requested a review from rohit-nayak-ps as a code owner January 22, 2022 18:07

GuptaManan100 mentioned this pull request Jan 22, 2022

DO NOT MERGE - Semi sync durability policy final check #9552

Closed

3 tasks

GuptaManan100 changed the title ~~Semi sync durability policy~~ Pass Semi sync information from durability policy and use it to log discrepancies Jan 22, 2022

shlomi-noach reviewed Jan 24, 2022

View reviewed changes

go/vt/orchestrator/inst/tablet_dao.go Outdated Show resolved Hide resolved

go/vt/vttablet/tabletmanager/rpc_actions.go Outdated Show resolved Hide resolved

go/vt/vttablet/tabletmanager/rpc_actions.go Outdated Show resolved Hide resolved

deepthi reviewed Jan 25, 2022

View reviewed changes

deepthi approved these changes Jan 25, 2022

View reviewed changes

GuptaManan100 added 4 commits January 25, 2022 18:45

refactor: rename ReplicaSemiSync to IsReplicaSemiSync

41acb37

Signed-off-by: Manan Gupta <manan@planetscale.com>

refactor: rename SemiSyncActions values

a5d5e17

Signed-off-by: Manan Gupta <manan@planetscale.com>

refactor: rename durability_policy flag

411a983

Signed-off-by: Manan Gupta <manan@planetscale.com>

refactor: add comment explaining the use of isPrimarySideSemiSyncEnabled

79d2822

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 merged commit f6ba0dd into vitessio:main Jan 25, 2022

GuptaManan100 deleted the semi-sync-durability-policy branch January 25, 2022 15:59

This was referenced Jan 27, 2022

Add multiple changes in release docs #9570

Merged

Add docs for the durability policies vitessio/website#955

Merged

Add durability flag in local examples #9579

Merged

GuptaManan100 mentioned this pull request Jul 18, 2022

Deprecate enable-semi-sync in favour of RPC parameter #10695

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass Semi sync information from durability policy and use it to log discrepancies #9533

Pass Semi sync information from durability policy and use it to log discrepancies #9533

GuptaManan100 commented Jan 19, 2022 •

edited

Loading

GuptaManan100 commented Jan 22, 2022

shlomi-noach left a comment

deepthi Jan 25, 2022

deepthi Jan 25, 2022

doeg Jan 27, 2022 •

edited

Loading

deepthi Jan 25, 2022

deepthi Jan 25, 2022

notfelineit Jan 27, 2022

deepthi Jan 27, 2022

notfelineit Jan 27, 2022

deepthi Jan 27, 2022

notfelineit Jan 27, 2022

notfelineit Feb 1, 2022 •

edited

Loading

deepthi left a comment

Pass Semi sync information from durability policy and use it to log discrepancies #9533

Pass Semi sync information from durability policy and use it to log discrepancies #9533

Conversation

GuptaManan100 commented Jan 19, 2022 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

GuptaManan100 commented Jan 22, 2022

shlomi-noach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doeg Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notfelineit Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

deepthi left a comment

Choose a reason for hiding this comment

GuptaManan100 commented Jan 19, 2022 •

edited

Loading

doeg Jan 27, 2022 •

edited

Loading

notfelineit Feb 1, 2022 •

edited

Loading