Add options params to buildMutualTlsChannel()

[renjiezh]

This change is 

[SanjayVas]

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @renjiezh and @SanjayVas)

a discussion (no related file):
Can you explain why you'd want to do this? I don't know of any channel options we'd want to set globally.

---

---

imports/java/io/netty/transport/BUILD.bazel line 4 at r1 (raw file):

alias(
    name = "transport",

The package in the //imports should match the Java package. That implies that this library only contains symbols in the io.netty.transport Java package, but it seems like this is being used to import io.netty.channel.ChannelOption.

[renjiezh]

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @SanjayVas)

a discussion (no related file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Can you explain why you'd want to do this? I don't know of any channel options we'd want to set globally.

This is for service_config. We need to pass the config via channelOption.
https://www.retinadata.com/blog/configuring-grpc-retries/

---

---

imports/java/io/netty/transport/BUILD.bazel line 4 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The package in the //imports should match the Java package. That implies that this library only contains symbols in the io.netty.transport Java package, but it seems like this is being used to import io.netty.channel.ChannelOption.

io.netty.channel is in the package io.netty.transport.
https://github.com/netty/netty/tree/4.1/transport/src/main/java/io/netty/channel
Any alternative way to import it?

[SanjayVas]

Reviewed 1 of 3 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @renjiezh)

a discussion (no related file):

Previously, renjiezh wrote…

This is for service_config. We need to pass the config via channelOption.
https://www.retinadata.com/blog/configuring-grpc-retries/

I'm not sure this is the standard way to specify the service config. My understanding is that the config is usually published by the service rather than specified by the client. https://github.com/grpc/grpc/blob/master/doc/service_config.md mentions that the config is usually returned by a name resolver plugin. I'm not sure if that's tricky to set up and/or not applicable to our use case, but that should be investigated.

---

---

imports/java/io/netty/transport/BUILD.bazel line 4 at r1 (raw file):

Previously, renjiezh wrote…

io.netty.channel is in the package io.netty.transport.
https://github.com/netty/netty/tree/4.1/transport/src/main/java/io/netty/channel
Any alternative way to import it?

I think you're mixing up Java package and Maven artifact. io.netty.channel is the package.

• If a Maven artifact is the only one which includes symbols for a given Java package, we put the target in the BUILD file under the Bazel package path that matches the Java package and give the target the same name as the Bazel package.
• If there are multiple Maven artifacts that include symbols from the same Java package, then the target for each artifact should be in the BUILD file under the Bazel package path that matches the lowest common Java package. The target name generally references the artifact name in some way, and does not take on the name of the Bazel package.

In this case, it looks like the netty-transport Maven artifact includes symbols in both io.netty.channel and io.netty.bootstrap. Therefore, the Bazel package should be //imports/java/io/netty. I'd just call the target transport.

[SanjayVas]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @renjiezh)

a discussion (no related file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I'm not sure this is the standard way to specify the service config. My understanding is that the config is usually published by the service rather than specified by the client. https://github.com/grpc/grpc/blob/master/doc/service_config.md mentions that the config is usually returned by a name resolver plugin. I'm not sure if that's tricky to set up and/or not applicable to our use case, but that should be investigated.

I did some digging and it sounds like that's all related to service discovery and load balancing. It might be something we should set up in the future, but I suppose for now doing this in the client is the best approach.

---

[renjiezh]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)

a discussion (no related file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I did some digging and it sounds like that's all related to service discovery and load balancing. It might be something we should set up in the future, but I suppose for now doing this in the client is the best approach.

Thank you sanjay. The service_config on the client side is like a default value when the server provides none. It looks good to me.

---

---

imports/java/io/netty/transport/BUILD.bazel line 4 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I think you're mixing up Java package and Maven artifact. io.netty.channel is the package.

• If a Maven artifact is the only one which includes symbols for a given Java package, we put the target in the BUILD file under the Bazel package path that matches the Java package and give the target the same name as the Bazel package.
• If there are multiple Maven artifacts that include symbols from the same Java package, then the target for each artifact should be in the BUILD file under the Bazel package path that matches the lowest common Java package. The target name generally references the artifact name in some way, and does not take on the name of the Bazel package.

In this case, it looks like the netty-transport Maven artifact includes symbols in both io.netty.channel and io.netty.bootstrap. Therefore, the Bazel package should be //imports/java/io/netty. I'd just call the target transport.

Done.
Thanks for the explanation. I will be careful next time. This PR doesn't need the extra package now.

[SanjayVas]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @renjiezh)

[SanjayVas]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @renjiezh)

---

src/main/kotlin/org/wfanet/measurement/common/grpc/Channel.kt line 44 at r2 (raw file):

  if (serviceConfig != null) {
    channelBuilder.defaultServiceConfig(serviceConfig).enableRetry()

Apparently retries are enabled by default. This mechanism exists to allow you to disable retries if you want to ignore the service config.

See https://github.com/grpc/proposal/blob/master/A6-client-retries.md#disabling-retries

Code quote:

.enableRetry()