Modify the default grpc keepalive time parameter

[inolddays]

1.set grpc_keepalive_time to 10 seconds
2.set grpc_keepalive_timeout to 10 seconds
These settings will help to avoid the situation which query will hang there when pod or Bare metal is broken.

[sougou]

This looks safe to me, but let's have @rafael and @tirsen (or @mpawliszyn ) who are sensitive to grpc behavior changes have a look before we merge.

[rafael]

This particular change shouldn't create issues in our end as we already set values for this.

However, there is something I think it is worth calling out. We've been super conservative in setting default values for external dependencies. The rationale is that introducing this silently could create weird behavior for folks that were relying on default values of the library itself.

For instance, in this case we even add some extra logic in the case where this is zero to not pass the value at all: https://github.com/vitessio/vitess/blob/master/go/vt/grpcclient/client.go#L69

If we would like to change/reconsider this pattern and this we should discuss it more broadly as we use this approach in many other places in the code base.

Personally, for external libraries like grpc/s3/xtrabackup I prefer not setting any defaults from Vitess perspective and let users be explicit about it. We can come up with recommendations, but by default not set it.

[sougou]

I think @rafael has a point. However, VTGate hanging forever on a down tablet is a problem that still needs solving.

In reality, there are already specific changes in behavior that we make based on different use cases within vitess. For example, VTGate dials vttablet with the FailFast option, while the default behavior is not to fail fast. This change is required so that vtgate can quickly retry another tablet if the current one is down.

Along the same lines, I think VTGate should explicitly pass these flags, and I believe these should be the same as the healthcheck interval. In other words, there should be no need to export a separate value.

Based on my understanding, if the existing (global) flag is overridden, it will end up superseding the values supplied by vtgate (later values win). So, it should be a non-breaking change for anyone that's already overriding these values.

In other words, I think we should make the tablet dialer require these values on Dial, and then pass then through to grpcclient.Dial.

[morgo]

@Johnny-Three Can you rebase this PR on master so the testsuite passes? From the feedback, it looks like we should be good to merge this.

[inolddays]

close this and created 5922 to replace .

[inolddays]

close this and created 5922 to replace.