VReplication: Fix workflow update changed handling#15621
VReplication: Fix workflow update changed handling#15621mattlord merged 3 commits intovitessio:mainfrom
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
mattlord
added
Backport to: release-18.0
and removed
Backport to: release-18.0
NeedsDescriptionUpdate
mattlord
force-pushed
the
workflow_update
branch
from
6f22505 to
2965e9c
Compare
mattlord
requested review from
GuptaManan100,
deepthi,
notfelineit,
rohit-nayak-ps and
shlomi-noach
as code owners
mattlord
changed the title
This was inadvertently broken in vitessio#14447 Signed-off-by: Matt Lord <mattalord@gmail.com>
mattlord
force-pushed
the
workflow_update
branch
from
2965e9c to
8074898
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15621 +/- ##
==========================================
+ Coverage 68.03% 68.12% +0.09%
==========================================
Files 1561 1556 -5
Lines 195526 194984 -542
==========================================
- Hits 133023 132840 -183
+ Misses 62503 62144 -359 ☔ View full report in Codecov by Sentry. |
| TabletSelectionPreference: tsp, | ||
| OnDdl: binlogdatapb.OnDDLAction(onddl), | ||
| Shards: baseOptions.Shards, | ||
| State: binlogdatapb.VReplicationWorkflowState(textutil.SimulatedNullInt), // We don't allow changing this in the client command |
There was a problem hiding this comment.
If we're not using the cli, is there documentation for when the zero value isn't sufficient for a no-op? Does it make sense to at least add field level comments to the proto?
There was a problem hiding this comment.
That's helpful. Is it backwards compatible if we add optional to those fields?
There was a problem hiding this comment.
In the go protobuf implementation everything is optional. And unfortunately it does not support the more recent optional protobuf behavior where code is then injected to detect if a value was explicitly provided in the message. That's what the simulated NULL stuff is all about, because there's literally no way to determine if a value was actually provided or not (type's ZeroValue). I'm not terribly happy with it, but I could not find a less bad option today. I'd love to get rid of the simulated NULL whenever a better option appears.
There was a problem hiding this comment.
Sorry, my understanding is now dated! The go implementation DOES now support truly optional field behavior: https://github.com/protocolbuffers/protobuf/blob/v3.25.0/docs/field_presence.md
It does that by making the specified type, e.g. string, a pointer (*string). I'm looking into whether or not that's backward compatible now.
There was a problem hiding this comment.
Added a TODO in the comments as well to link everything together.
1. Add comments to the workflow update related proto messages 2. Remove/reserve an unused shards field in the WorkflowUpdateRequest msg 3. Add a wait to the mulit_tenant_test to address an obvserved flake Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
| for _, ks := range []string{sourceKeyspace, sourceAliasKeyspace} { | ||
| lastIndex = insertRows(lastIndex, ks) | ||
| } | ||
| waitForWorkflowState(t, vc, fmt.Sprintf("%s.%s", targetKeyspace, mt.workflowName), binlogdatapb.VReplicationWorkflowState_Running.String()) |
There was a problem hiding this comment.
This is unrelated, but addresses a test flake that was observed on the PR CI runs.
| binlogdata.OnDDLAction on_ddl = 5; | ||
| binlogdata.VReplicationWorkflowState state = 6; | ||
| repeated string shards = 7; | ||
| reserved 7; // unused, was: repeated string shards |
There was a problem hiding this comment.
This was errantly added in v19 and was missed during review. It caused no harm, but was entirely unnecessary/unused.
3 participants
Description
The rows affected value in the
UpdateVReplicationWorkflowtabletmanager RPC (see the PR changes) which is then turned into a booleanchangedvalue in the correspondingUpdateWorkflowvtctld RPC was inadvertently broken in: #14447This PR corrects that and also addresses an oversight in adding support for changing the State in the tabletmanager RPC (we should be passing the simulated NULL for the client command as we don't allow you to modify the state via the
workflow updateclient command). Note that this had no real practical impact as the workflow is stopped and started as part of the update so the intermediate state ofUnknownwas extremely brief.Lastly, we close the testing gap here by confirming the changed value from the client command.
I wanted to backport this to v18 as it's a small and safe change and that's where these bugs were introduced.
Related Issue(s)
Checklist