-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
PRS and ERS don't promote replicas taking backups #16997
PRS and ERS don't promote replicas taking backups #16997
Conversation
Signed-off-by: Eduardo J. Ortega U <[email protected]>
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
|
Signed-off-by: Eduardo J. Ortega U <[email protected]>
Signed-off-by: Eduardo J. Ortega U <[email protected]>
Signed-off-by: Eduardo J. Ortega U <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16997 +/- ##
==========================================
+ Coverage 67.31% 67.32% +0.01%
==========================================
Files 1569 1570 +1
Lines 252502 252762 +260
==========================================
+ Hits 169964 170182 +218
- Misses 82538 82580 +42 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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.
It would be good to include an end-to-end test for this covering several cases: only 1 replica in the cluster, and more than one replica, all replicas being backed up, etc.
Moreover, even though this is not a breaking change per se, we should still document it in the v22.0
release notes. Which should be put in ./changelog/22.0/22.0.0/summary.md
, the file does not exist yet.
Have PRS remove hosts taking backups from consideration; and ERS only consider them if there are no other valid candidates that are not taking backups. Signed-off-by: Eduardo J. Ortega U <[email protected]>
Signed-off-by: Eduardo J. Ortega U <[email protected]>
@@ -58,7 +58,8 @@ const ( | |||
// cell as the current primary, and to be different from avoidPrimaryAlias. The | |||
// tablet with the most advanced replication position is chosen to minimize the | |||
// amount of time spent catching up with the current primary. Further ties are | |||
// broken by the durability rules. | |||
// broken by the durability rules. Tablets taking backups are excluded from |
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.
@ejortegau I think this comment is no longer accurate
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.
No, it's still accurate. It reflects the fact that PRS will not promote a tablet that is taking a backup.
I have updated the PR to do the following:
Please have another look. |
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.
Just one comment change, rest looks good to me!
Signed-off-by: Eduardo J. Ortega U <[email protected]>
Thanks for the contribution and for your patience with the review process. |
Signed-off-by: Eduardo J. Ortega U <[email protected]> Signed-off-by: Renan Rangel <[email protected]>
Signed-off-by: Eduardo J. Ortega U. <[email protected]>
Signed-off-by: Eduardo J. Ortega U <[email protected]>
Signed-off-by: Eduardo J. Ortega U. <[email protected]>
* PRS and ERS don't promote replicas taking backups (vitessio#16997) Signed-off-by: Eduardo J. Ortega U <[email protected]>
Description
This PR changes ERS and PRS so that they prefer not promoting hosts that are currently taking backups.
The implementation follows what was suggesteed in the RFC of the issue (link below). Namely, the RPCs used to get information about candidates now include an extra field indicating whether they are running backups or not; and that is
used to order the list of promotion candidates.
Related Issue(s)
#16558
Checklist
Deployment Notes
N/A