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.

Sign up for GitHub

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

Updates enforcement policy to match the one from the client #6629

Merged
merged 1 commit into from
Aug 26, 2020
Merged

Updates enforcement policy to match the one from the client #6629

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go/vt/servenv/grpc_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ var (

// EnforcementPolicy MinTime that sets the keepalive enforcement policy on the server.
// This is the minimum amount of time a client should wait before sending a keepalive ping.
GRPCKeepAliveEnforcementPolicyMinTime = flag.Duration("grpc_server_keepalive_enforcement_policy_min_time", 5*time.Minute, "gRPC server minimum keepalive time")
GRPCKeepAliveEnforcementPolicyMinTime = flag.Duration("grpc_server_keepalive_enforcement_policy_min_time", 10*time.Second, "gRPC server minimum keepalive time")
Copy link
Member

Choose a reason for hiding this comment

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

I think in addition to changing the default, we should also force the value if it violates the constraint we want to impose (that PolicyMinTime <= grpc_keepalive_timeout). Line 157.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I'm not sure if it should be up to us to do this kind of validations. I think that could be a slippery slope. Two things come to mind:

  • I think we already have many other settings that could create conflict if not configured correctly that we are not doing this.
  • Even if we add that validation we can't prevent that vtgates are configured incorrectly in respect to the flags that the user provides to the tablet (e.g you could configure the vtgates to be 10s and enforcement policy of 10s; then tablets to be 5s / 5s. The constraint you are proposing won't catch that.

Copy link
Member

Choose a reason for hiding this comment

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

Valid points. We can merge this as-is.


// EnforcementPolicy PermitWithoutStream - If true, server allows keepalive pings
// even when there are no active streams (RPCs). If false, and client sends ping when
Expand Down