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-se ... (#17885) #17944

Merged
merged 3 commits into from
Jun 28, 2020

Conversation

sre-bot
Copy link
Contributor

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

cherry-pick #17885 to release-3.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

@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

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

@qw4990 qw4990 added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. labels Jun 28, 2020
@ti-srebot
Copy link
Contributor

Sorry @qw4990, you don't have permission to trigger auto merge event on this branch.

@zz-jason zz-jason changed the title tikv: fix "Got too many pings" GRPC error in PD-server follower (#17885) tikv: fix "Got too many pings" GRPC error in PD-se ... (#17885) Jun 28, 2020
@zz-jason zz-jason merged commit 82846dd into pingcap:release-3.0 Jun 28, 2020
@@ -56,7 +56,6 @@ func createEtcdKV(addrs []string, tlsConfig *tls.Config) (*clientv3.Client, erro
Endpoints: addrs,
AutoSyncInterval: 30 * time.Second,
DialTimeout: 5 * time.Second,
TLS: tlsConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this removal of TLS field intentional?

@lysu @jackysp @zimulala cc @3pointer

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is an accident...

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.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants