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.

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

fix hang when local topo was down #7278

Merged

Conversation

inolddays
Copy link
Contributor

@inolddays inolddays commented Jan 10, 2021

when local cell topo server is down, all commands related with function GetSrvKeyspaceNames are hanging
Added topo timeout and when local cell topo is broken , will use cache instead , make sure not too much effect on vtgate

Signed-off-by: JohnnyThree [email protected]

Description

Related Issue(s)

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build
  • VTAdmin

@inolddays inolddays requested a review from sougou as a code owner January 10, 2021 07:56
@inolddays inolddays force-pushed the fixlocaltopobrokeshowdatabasehang branch from 1fde084 to 32c8a64 Compare January 10, 2021 15:07
@deepthi deepthi requested a review from rafael January 12, 2021 18:25
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.

Looks fine except for the one comment.

@@ -308,7 +310,8 @@ func (server *ResilientServer) GetSrvKeyspaceNames(ctx context.Context, cell str
server.counts.Add(errorCategory, 1)
if entry.insertionTime.IsZero() {
log.Errorf("GetSrvKeyspaceNames(%v, %v) failed: %v (no cached value, caching and returning error)", ctx, cell, err)

} else if strings.Contains(err.Error(), "deadline exceeded") {
Copy link
Member

@deepthi deepthi Jan 19, 2021

Choose a reason for hiding this comment

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

It will be better to check

newCtx.Err() == context.DeadlineExceeded

instead of checking for the error string.

Copy link
Contributor Author

@inolddays inolddays Jan 20, 2021

Choose a reason for hiding this comment

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

gotcha, done

@inolddays inolddays force-pushed the fixlocaltopobrokeshowdatabasehang branch from 32c8a64 to ebd8c60 Compare January 20, 2021 03:24
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants