-
Notifications
You must be signed in to change notification settings - Fork 11
[Do Not Merge] v20.x #152
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
base: release-20.0
Are you sure you want to change the base?
[Do Not Merge] v20.x #152
Conversation
…gate startup (vitessio#16655) Signed-off-by: Manan Gupta <[email protected]> Signed-off-by: Arthur Schreiber <[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.
Pull Request Overview
This PR updates the Vitess tablet gateway and keyspace event watcher to gate startup based on keyspace consistency and buffering conditions during reparent operations. Key changes include:
- Replacing the legacy PrimaryIsNotServing API with the more descriptive ShouldStartBufferingForTarget in both production and test code.
- Introducing WaitForConsistentKeyspaces to ensure keyspaces are processed before allowing primary tablet queries.
- Updating srvtopo discovery functions to return both targets and keyspaces.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
go/vt/vtgate/tabletgateway_test.go | Added a new test (TestWithRetry) that validates the new retry and buffering behavior. |
go/vt/vtgate/tabletgateway_flaky_test.go | Updated tests to use ShouldStartBufferingForTarget instead of PrimaryIsNotServing. |
go/vt/vtgate/tabletgateway.go | Replaced API calls to use the new keyspace event buffering logic and await keyspace consistency. |
go/vt/srvtopo/discover_test.go | Renamed tests to use FindAllTargetsAndKeyspaces and assert on the returned keyspaces list. |
go/vt/srvtopo/discover.go | Updated function signature and documentation to return both targets and keyspaces. |
go/vt/discovery/keyspace_events_test.go | Adjusted tests to validate the new ShouldStartBufferingForTarget API and its expectations. |
go/vt/discovery/keyspace_events.go | Renamed PrimaryIsNotServing to ShouldStartBufferingForTarget and added WaitForConsistentKeyspaces. |
Comments suppressed due to low confidence (1)
go/vt/discovery/keyspace_events.go:749
- [nitpick] The compound boolean expression is complex; consider refactoring it into multiple well-named boolean variables or adding inline comments to improve readability and maintainability.
return state.currentPrimary, !state.serving && !ks.consistent && state.externallyReparented != 0 && state.currentPrimary != nil
func (kew *KeyspaceEventWatcher) WaitForConsistentKeyspaces(ctx context.Context, ksList []string) error { | ||
// We don't want to change the original keyspace list that we receive so we clone it | ||
// before we empty it elements down below. | ||
keyspaces := slices.Clone(ksList) |
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.
WaitForConsistentKeyspaces uses a fixed sleep interval in a loop which may lead to long waits if keyspaces never become consistent; consider introducing a configurable timeout or maximum iteration limit to avoid potential resource waste.
func (kew *KeyspaceEventWatcher) WaitForConsistentKeyspaces(ctx context.Context, ksList []string) error { | |
// We don't want to change the original keyspace list that we receive so we clone it | |
// before we empty it elements down below. | |
keyspaces := slices.Clone(ksList) | |
func (kew *KeyspaceEventWatcher) WaitForConsistentKeyspaces(ctx context.Context, ksList []string, timeout time.Duration) error { | |
// We don't want to change the original keyspace list that we receive so we clone it | |
// before we empty it elements down below. | |
keyspaces := slices.Clone(ksList) | |
timeoutChan := time.After(timeout) |
Copilot uses AI. Check for mistakes.
) (#149) * VReplication: Estimate lag when workflow fully throttled (vitessio#16577) Signed-off-by: Matt Lord <[email protected]> * remove check result summary * rerun actions Signed-off-by: Mohamed Hamza <[email protected]> * fix runs on in workflows * update actions --------- Signed-off-by: Matt Lord <[email protected]> Signed-off-by: Mohamed Hamza <[email protected]> Co-authored-by: Matt Lord <[email protected]>
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
This PR was closed because it has been stale for 7 days with no activity. |
…0-github Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: David Piegza <[email protected]>
Signed-off-by: Dirkjan Bussink <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Fix formatting for table entries in select_cases.json
…ableACLConfig is true (vitessio#17274) (#173) Signed-off-by: garfthoffman <[email protected]> Signed-off-by: Mohamed Hamza <[email protected]> Co-authored-by: garfthoffman <[email protected]>
…essio#16577) (#149)" This reverts commit 9815ca8.
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
) Signed-off-by: Matt Lord <[email protected]>
sqlparser: Remove unneeded escaping (vitessio#16255)
VReplication: Estimate lag when workflow fully throttled (vitessio#16577)
Description
This pull request is used to track which backports we've done against the upstream v20 release.
Related Issue(s)
Checklist
Deployment Notes