vitessdriver: add ability to set custom gRPC dial options

[MordFustang21]

This will allow users to set custom gRPC dial options in the driver configuration. We needed this so we could increase the max message receive size.

Signed-off-by: Derrick Laird swampdonk@gmail.com

[derekperkins]

This works in our environment. We specifically needed it to set the max message size to pair with the partner flags on vtgate and vttablet. I imagine we'll be trying more options as we start perf testing.

[sougou]

Sorry about the delay on this due to travel.

The approach you're using is not what I would have thought, but I like it.

What I would have done:

• Add a new function in grpcvtgateconn: AddDialOptions, and it would just save the opts to a global dialOptions variable.
• dial will use dialOptions when invoked.
• The app would directly invoke grpcvtgateconn.AddDialOptions before invoking Configuration.Open.

The above approach keeps the driver agnostic of the protocol. The downside is that the app requires that extra invocation. It also feels a little hacky.

Your approach is more of an enhancement of the grpc protocol. The way you should modify this: create a new protocol. Something like grpc_with_options. This should be a separate file that you can add-on as a plugin, and it need not be part of the vitess build.

The reason why I like this approach is that it follows the full spirit of the driver being agnostic of the protocol. The app doesn't need to set the options, because you can set them upfront in the dialer. The downside is that it will be a custom build.

Now that I think about it, the second will be a problem for you because you pull images directly from docker. More thinking is needed for this.

[derekperkins]

it would just save the opts to a global dialOptions variable

We talked about implementing a global var, but I generally try to avoid them. I don't have a specific use case in mind, but it doesn't seem out of the question that you might set different defaults for different connection types.

The above approach keeps the driver agnostic of the protocol. The downside is that the app requires that extra invocation. It also feels a little hacky.

When we were thinking about it, we were trying to make something discoverable for anyone using the driver. I can't think of any reason somebody would use something other than grpc if they are using this package. If they wanted to use the mysql protocol, they would just use a mysql driver. I understand your separation of concerns in theory, but in practice, I think making the options for the default protocol more discoverable and usable is the right choice.

We were trying to wrap the weird registration details on purpose. I think Go's flag system is fundamentally flawed because you shouldn't be passing flags to your own binaries to set a default in an imported database driver. YouTube must have been explicitly setting the protocol in the configuration struct, otherwise they would have still been forced to use grpc due to the default override.

Now that I think about it, the second will be a problem for you because you pull images directly from docker. More thinking is needed for this.

This doesn't actually impact us because we're importing the vitessdriver in our builds and that is compiling into our app since these are just client level options.

All that being said, I think the implementation as written is the best of the 3 options, however if you'd prefer us to do it another way, pick one and @MordFustang21 will make it happen. It's a pretty minimal amount of code whatever we decide.

[sougou]

Sounds good. Feel free to approve and merge if there's no further work on this.

[derekperkins]

We cleaned it up a little more and added some more comments to the code to be explicit about choices that were made, including some of the original behavior of toJSON(). We're testing on our systems and will merge once that's complete. Thanks for the feedback!