Skip to content
Snippets Groups Projects

Revert "Merge branch 'wc-gitaly-keepalive-limit' into 'master'"

Merged Stan Hu requested to merge revert-b574ddd8 into master
All threads resolved!

What does this MR do and why?

This reverts !73302 (merged). This setting causes long-running RPCs to shut down and causes the client to receive GOAWAY messages from the server. As explained in https://github.com/grpc/grpc/issues/25713, configuring Gitaly keepalive settings is dangerous: if the client sends too many, then the server will abruptly shut down the connection, causing RPCs to fail.

At the moment, it doesn't appear grpc-go has a way to configure or even disable this shutdown mechanism (I submitted https://github.com/grpc/grpc-go/pull/5162). For now, we should revert this change because it does more harm than good.

Go 1.13 should have enabled 15-second TCP keepalives by default; I'm not sure yet why this isn't working, or whether gRPC is fiddling with this as well.

Relates to #350580 (closed)

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Stan Hu

Merge request reports

Merged results pipeline #454537463 passed with warnings

Merged results pipeline passed with warnings for e611f06f

Test coverage 86.98% (-1.35%) from 2 jobs
Approved by

Merged by Stan HuStan Hu 3 years ago (Jan 24, 2022 10:16pm UTC)

Pipeline #455342494 passed with warnings

Pipeline passed with warnings for 0693fbff on master

Test coverage 78.87% (-1.35%) from 2 jobs
8 environments impacted.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
Please register or sign in to reply
Loading