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

tikv: fix "Got too many pings" GRPC error in PD-server follower (#17885) #17947

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

sre-bot
Copy link
Contributor

@sre-bot sre-bot commented Jun 11, 2020

cherry-pick #17885 to release-4.0


What problem does this PR solve?

Issue Number: close #17870

Problem Summary:

In PD(etcd) server side, PermitWithoutStream values is false https://github.com/etcd-io/etcd/blob/master/embed/etcd.go#L641, means

and client sends ping when there are no active streams, server will send GOAWAY and close the connection.

but in TiDB client side, PermitWithoutStream values is true means

client sends keepalive pings even with no active RPCs

so, with those conflict configuration, grpc server side will count pingStrike with wrong value.

https://github.com/grpc/grpc-go/blob/master/internal/transport/http2_server.go#L711

and report "transport: Got too many pings from the client, closing the connection." error.

What is changed and how it works?

What's Changed, How it Works:

we need keep them both as true or false, but etcd write it with magic value(false)

so we only can change tidb-side to false

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Manual test
setup a cluster, some tidb node use this fix
"Got too many pings" will happen but only in node that without this patch.

Side effects

  • ?

Release note

  • Fix "Got too many pings" grpc error log in PD-server follower

This change is Reviewable

Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor Author

sre-bot commented Jun 11, 2020

/run-all-tests

Copy link
Member

@coocood coocood left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Jun 11, 2020
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp jackysp removed the sig/sql-infra SIG: SQL Infra label Jun 11, 2020
@jackysp
Copy link
Member

jackysp commented Jun 11, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 11, 2020
@sre-bot
Copy link
Contributor Author

sre-bot commented Jun 11, 2020

Your auto merge job has been accepted, waiting for:

  • 17899

@sre-bot
Copy link
Contributor Author

sre-bot commented Jun 11, 2020

/run-all-tests

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Jun 11, 2020
@sre-bot sre-bot merged commit e866e99 into pingcap:release-4.0 Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/tikv sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. type/bugfix This PR fixes a bug. type/4.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants