-
Notifications
You must be signed in to change notification settings - Fork 2.4k
vtorc: add support for dynamic enable/disable of ERS by keyspace/shard
#17985
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
Merged
timvaillancourt
merged 42 commits into
vitessio:main
from
timvaillancourt:vtorc-ks-topo-config
Sep 3, 2025
Merged
Changes from 33 commits
Commits
Show all changes
42 commits
Select commit
Hold shift + click to select a range
0aaf7d8
`vtorc`: add support for dynamic enable/disable of ERS by keyspace/shard
timvaillancourt a83ed22
Merge remote-tracking branch 'origin/main' into vtorc-ks-topo-config
timvaillancourt 0ffdf40
move new structs to new proto package
timvaillancourt 16c91e6
Merge remote-tracking branch 'origin/main' into vtorc-ks-topo-config
timvaillancourt 568ccc0
simplify changelog
timvaillancourt 8bef5c7
simplify changelog again
timvaillancourt d94bb21
update shard conditional
timvaillancourt 52a2245
update shard conditional, again
timvaillancourt ffbad5e
one more field rename
timvaillancourt 2cdba44
update proto comment
timvaillancourt 0d55c73
missing rename
timvaillancourt 96b0d3e
update `TestAPI`
timvaillancourt 601ab1f
Merge branch 'main' into vtorc-ks-topo-config
timvaillancourt 3841960
gofmt
timvaillancourt 890aeac
Merge remote-tracking branch 'origin/main' into vtorc-ks-topo-config
timvaillancourt cbbbd3b
revert .CreateShard signature change
timvaillancourt 50d8fe4
fix test failures
timvaillancourt f4612f6
fix test
timvaillancourt b75b70b
update protos
timvaillancourt 58e7df3
PR suggestion: rename field
timvaillancourt 5a98ae0
lint
timvaillancourt 5134083
test/changelog fix
timvaillancourt 7c6fc22
add e2e test
timvaillancourt efc0bab
rm metric check
timvaillancourt 9ec8abf
cleanup
timvaillancourt 624bb80
e2e fix
timvaillancourt 06dbe21
improve test
timvaillancourt 6591a49
use assert
timvaillancourt c9e639b
cleanup test
timvaillancourt 0e67548
update docs
timvaillancourt 7f6cd7c
major -> minor feature
timvaillancourt 66b12c2
rename test helper
timvaillancourt 900f4ac
rename test helper again
timvaillancourt f3b3718
PR suggestions, wait for skipped recovery
timvaillancourt ed0a892
add changelog for new metric
timvaillancourt f89a760
typo fix
timvaillancourt 32932f9
check primary after skipped recovery wait
timvaillancourt 89c7c47
missing rename
timvaillancourt 65cce15
rm dupe func
timvaillancourt bf5c551
move `SkippedRecoveries` to better place
timvaillancourt e824bd0
use `assert.EventuallyWithT(...)` for test helpers
timvaillancourt 4ea4fc4
fix typo
timvaillancourt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tricky. We wait for something to not happen and thus we provide
0for expected count. But if you look at the code forutils.WaitForSuccessfulRecoveryCountandutils.WaitForSuccessfulERSCount, these functions just loop with timeout until they reach requested value, which is0, which means they will exit immediately with success code.Point being, if a failover was to happen, we'd still exit immediately, and this is wrong. I think in this case we need to add an imposed
Sleepof a few seconds (you call the number) before calling these functions.Or, otherwise, find a more formal way of knowing for sure that the recovery was ignored. But
Sleepshould be fine.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shlomi-noach great point, I missed that! 👍. Yes, a sleep would work
Another way to solve this would be to wait for another metric: I'm thinking the "# of recoveries" counter, to increase for the
DeadPrimaryrecovery AND only then should the logic check if the desired value was reachedAn alternative idea, which I pondered adding in this PR, was a "# of ERS-recoveries skipped" metric
I think I'll try this 2-dimensional metrics check and see where it goes! If a sleep approach is necessary, that's an option too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that have the same behavior? You want that number to not incrase so the only way to check that would be to wait/sleep?
That would be good indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shlomi-noach I've updated the e2e test to wait for the
RecoverDeadPrimaryrecovery to be skipped, using a new metric:SkippedRecoveriesOnce we know
RecoverDeadPrimarywas skipped, it should be safe to check for0for the stats you originally pointed out. Let me know how it looks! 🙇