-
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
Tablet picker: Handle the case where a primary tablet is not setup for a shard #17573
Tablet picker: Handle the case where a primary tablet is not setup for a shard #17573
Conversation
Signed-off-by: Rohit Nayak <[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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17573 +/- ##
========================================
Coverage 67.71% 67.71%
========================================
Files 1584 1585 +1
Lines 254721 254833 +112
========================================
+ Hits 172473 172562 +89
- Misses 82248 82271 +23 ☔ View full report in Codecov by Sentry. |
// there is no primary setup for the shard we correctly return an error. | ||
func TestPickNoPrimary(t *testing.T) { | ||
defer utils.EnsureNoLeaks(t) | ||
ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) |
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.
Why use multiple contexts, and why with such short timeouts? Won't this make it flaky in the CI? I get why we use the short context for PickForStreaming()
— to cut short the search — but I don't see why we use the other two contexts (instead of one) with such short timeouts.
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 was a cut/paste of a previous test and I had only changed one line, so didn't focus on that. Changed to use a single context.
Signed-off-by: Rohit Nayak <[email protected]>
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.
LGTM, thanks!
…r a shard (#17573) Signed-off-by: Rohit Nayak <[email protected]>
…r a shard (#17573) Signed-off-by: Rohit Nayak <[email protected]>
…r a shard (#17573) Signed-off-by: Rohit Nayak <[email protected]>
…is not setup for a shard (#17573) (#17574) Signed-off-by: Rohit Nayak <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
…is not setup for a shard (#17573) (#17575) Signed-off-by: Rohit Nayak <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-by: Rohit Nayak <[email protected]>
…is not setup for a shard (#17573) (#17576) Signed-off-by: Rohit Nayak <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
Description
During a VReplication workflow, if the workflow has its
tablet_types
setup asprimary
, it would panic. This PR fixes that.This was added a while back, so we should backport to supported versions: https://github.com/vitessio/vitess/pull/14224/files#diff-1bcb2314a21db237d254efff723def4a78db39c34298592d0b0a9a72a58ef575R372
Related Issue(s)
#17571
Checklist
Deployment Notes