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) #17946

Merged
merged 4 commits into from
Jul 21, 2020

Conversation

sre-bot
Copy link
Contributor

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

cherry-pick #17885 to release-3.1


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

@jackysp jackysp removed 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

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

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added the status/LGT2 Indicates that a PR has LGTM 2. label Jun 12, 2020
@zz-jason
Copy link
Member

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 20, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@sre-bot merge failed.

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@sre-bot merge failed.

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@sre-bot merge failed.

@jackysp
Copy link
Member

jackysp commented Jul 21, 2020

/rebuild

@lysu
Copy link
Contributor

lysu commented Jul 21, 2020

/run-unit-test

@jackysp
Copy link
Member

jackysp commented Jul 21, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 1fb3c7c into pingcap:release-3.1 Jul 21, 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. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug. type/3.1-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants