-
Notifications
You must be signed in to change notification settings - Fork 90
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.
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
feat: Add universe domain support for Java #1904
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think the change LGTM from my end. Were you able test this locally and connect properly?
Yes, I was able to successfully publish and subscribe when setting the |
@@ -717,7 +718,8 @@ public static final class Builder { | |||
static final long DEFAULT_COMPRESSION_BYTES_THRESHOLD = 240L; | |||
|
|||
String topicName; | |||
private String endpoint = PublisherStubSettings.getDefaultEndpoint(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still being used somewhere? I think it's autogenerated so it can't be removed IIRC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line was added when endpoint overriding was enabled for the Publisher
/Subscriber
here. If a user sets the endpoint when building the Publisher
, we propagate that to the PublisherStubSettings builder
, which is autogenerated. Otherwise, PublisherStubSettings
uses the getDefaultEndpoint()
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add a bit more context from the GAPICs perspective. Gax-Java now will resolve the endpoint automatically so there is no need to set a default endpoint. One of the parameters that we take into consideration is the user set endpoint which is configured from the Builder's setEndpoint()
. All these factors (i.e. mtls, user set endpoint, user set universe domain, and more) will be considered when creating the final endpoint to be used.
The reason why we're suggesting to not pass the getDefaultEndpoint
to Gax is that Gax-Java won't be able to determine if the endpoint passed in is a user configuration or not. The new endpoint logic has user configuration override everything so it will always default to pubsub.googleapis.com
if it is passed in.
This adds a
setUniverseDomain()
method for thePublisher
andSubscriber
class so that users can easily set the universe domain, instead of having to create aStubSettings
object. Additionally, this sets the default endpoint in thePublisher
/Subscriber
builders to null, which will still resolve to the default endpoint.Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes: N/A ☕️
If you write sample code, please follow the samples format.