Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixes long wait filter keyspace #6721

Merged
merged 6 commits into from
Sep 18, 2020
Merged

Fixes long wait filter keyspace #6721

merged 6 commits into from
Sep 18, 2020

Conversation

rafael
Copy link
Member

@rafael rafael commented Sep 14, 2020

Description

  • The following PR fixes an issue with KeyspacesToWatch functionality. When this flag is provided, some keyspaces are removed. However, vtgates still try to wait for available tablet types in them. This PR makes sure that filter is respected.
  • I also added some tests around WaitForTablets for existent and new functionality.

@rafael
Improve logging for missing targets
de9ff79 
Signed-off-by: Rafael Chacon <rafael@slack-corp.com>
@rafael
Fixes discovery gateway issue where it was waiting for keyspaces that…
5a56d68 
… should be filtered

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>
@systay systay removed their request for review September 15, 2020 13:47
@deepthi
Copy link
Member

deepthi commented Sep 15, 2020

do you know whether this also needs to be fixed on the new gateway (TabletGateway)?

@deepthi deepthi self-requested a review September 15, 2020 17:53
@rafael rafael force-pushed the fixes-long-wait-filter-keyspace-upstream branch from f024ae1 to dbfe684 Compare September 15, 2020 19:28
@rafael
Also implement this bug fix inside the new tabletgateway implementation
0d66346 
Signed-off-by: Rafael Chacon <rafael@slack-corp.com>
@rafael rafael force-pushed the fixes-long-wait-filter-keyspace-upstream branch from dbfe684 to 0d66346 Compare September 17, 2020 17:39
@rafael
Merge branch 'master' of https://github.com/vitessio/vitess into fixe…
f852b28 
…s-long-wait-filter-keyspace-upstream

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>
deepthi
deepthi reviewed Sep 17, 2020
View reviewed changes
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation feels a bit fragile. We are mutating the arguments passed to an exported function. Even though existing uses of that function are such that this won't cause problems, given that it is an exported function, I feel that it is not a good idea.
How about creating a new slice that consists of only the relevant keyspaces and returning that instead of operating directly on targets?

go/vt/discovery/healthcheck.go Outdated Show resolved Hide resolved
go/vt/discovery/healthcheck.go Outdated Show resolved Hide resolved
go/vt/discovery/legacy_tablet_stats_cache_wait.go Outdated Show resolved Hide resolved
go/vt/vtgate/discoverygateway.go Outdated Show resolved Hide resolved
@rafael
Make this code less fragile and refactor to use the function in both …
fbb94b9 
…gateways

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>
@rafael
Copy link
Member Author

rafael commented Sep 17, 2020

The implementation feels a bit fragile. We are mutating the arguments passed to an exported function. Even though existing uses of that function are such that this won't cause problems, given that it is an exported function, I feel that it is not a good idea.
How about creating a new slice that consists of only the relevant keyspaces and returning that instead of operating directly on targets?

I think this is a good idea. I was on the fence because the existent code already mutates this same slice. I updated the code with your suggestion. We should probably do something similar here

@rafael
Improvements per linter
b238f25 
Signed-off-by: Rafael Chacon <rafael@slack-corp.com>
@rafael rafael force-pushed the fixes-long-wait-filter-keyspace-upstream branch from c8241ef to b238f25 Compare September 17, 2020 22:07
deepthi
deepthi approved these changes Sep 18, 2020
View reviewed changes
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@deepthi deepthi merged commit 9073c3e into vitessio:master Sep 18, 2020
ameetkotian pushed a commit to tinyspeck/vitess that referenced this pull request Sep 29, 2020
@deepthi @ameetkotian
Merge pull request vitessio#6721 from tinyspeck/fixes-long-wait-filte…
6b9229e 
…r-keyspace-upstream

Fixes long wait filter keyspace
@ameetkotian ameetkotian mentioned this pull request Sep 29, 2020
Closed
rafael added a commit to tinyspeck/vitess that referenced this pull request Oct 1, 2020
@rafael
Revert "Merge pull request vitessio#6721 from tinyspeck/fixes-long-wa…
6dfdfdc 
…it-filter-keyspace-upstream"

This reverts commit 6b9229e.
ameetkotian pushed a commit to tinyspeck/vitess that referenced this pull request Oct 1, 2020
@deepthi @ameetkotian
Merge pull request vitessio#6721 from tinyspeck/fixes-long-wait-filte…
b8901b9 
…r-keyspace-upstream

Fixes long wait filter keyspace
@askdba askdba added this to the v8.0 milestone Oct 6, 2020
@setassociative setassociative mentioned this pull request Mar 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepthi deepthi deepthi approved these changes

@harshit-gangal harshit-gangal Awaiting requested review from harshit-gangal

@sougou sougou Awaiting requested review from sougou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v8.0
Development

Successfully merging this pull request may close these issues.
3 participants
@rafael @deepthi @askdba