feat: Parse Host Service Name #2300
Conversation
gcf-owl-bot
bot
removed
the
owlbot:run
| @@ -100,6 +100,10 @@ public abstract class ClientContext { | |||
| @Nonnull | |||
| public abstract Duration getStreamWatchdogCheckInterval(); | |||
|
|
|||
| // Package-Private scope for internal use only. Shared between StubSettings and ClientContext | |||
| @Nullable | |||
| abstract String getHostServiceName(); | |||
There was a problem hiding this comment.
Shall we call this field serviceName in Gax and generated stubs? It makes sense to call ithostServiceName in the generator as serviceName could be confused with Service.name(), but in Gax I think it makes more sense to just call it serviceName.
There was a problem hiding this comment.
Sure, I'll rename to serviceName.
| @@ -52,6 +52,12 @@ public boolean hasDescription() { | |||
| return !Strings.isNullOrEmpty(description()); | |||
| } | |||
|
|
|||
| public String hostServiceName() { | |||
| // Default Host is guaranteed to exist and be non-null and non-empty | |||
There was a problem hiding this comment.
I remember we discussed that the service name could come from the proto or service yaml, is that still true? Or service name will always come from defaultHost in the proto?
There was a problem hiding this comment.
Ah, comment is misleading. I meant to write that the hostServiceName is guaranteed as the generator won't generate a library if it doesn't exist.
I remember we discussed that the service name could come from the proto or service yaml, is that still true?
It is still true. Checks the proto's default_host value first, then the service yaml file, and fails otherwise
| // This meant to be used internally by GAPIC clients to configure an endpoint. | ||
| // Users should avoid settings this as it could change their endpoint. | ||
| @InternalApi | ||
| public B setHostServiceName(String hostServiceName) { |
There was a problem hiding this comment.
Do we have to expose a setter for it? Since the host service name is a fixed String, I think we should implement it in a similar way as the to-be-deprecated default endpoint, that we generate a getter that returns the default host service name.
There was a problem hiding this comment.
I see, I think that makes sense. I'll update it.
| // Intended for Internal Use and Overriden by generated ServiceStubSettings classes. | ||
| // Meant to be shared between StubSettings and ClientContext. | ||
| @InternalApi | ||
| public String getServiceName() { | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
ClientContext.create() takes in the root StubSettings (not the subclasses). Create a default method that generated subclasses can override.
...va/com/google/api/generator/gapic/composer/grpc/goldens/DeprecatedServiceStubSettings.golden
Outdated
Show resolved
Hide resolved
| // or service yaml file. For Google Cloud Services, the default host value | ||
| // is expected to contain `.googleapis.com`. Exceptions may exist (i.e. localhost), | ||
| // in which case we will return an empty string. | ||
| private static String parseHostServiceName(String defaultHost) { |
There was a problem hiding this comment.
I know we are already testing this method indirectly through golden tests. Since this is in Service not composers, and we can easily mock the input, it would be great if we can have unit tests testing it directly as well.
There was a problem hiding this comment.
Added some tests for parsing, thanks!
|
|
Changes:
Generator parses the Default Host value to get the Host Service Name. The parsed value is passed to the {Service}StubSettings and then passed to the EndpointContext.
HostServiceName is either set to be package-private scope or marked with
@InternalApias it's meant for usage by GAPIC clients.