-
Notifications
You must be signed in to change notification settings - Fork 55
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
refactor: Add EndpointContext #2275
Conversation
Can we integrate EndpointContext into the existing code and make sure all functionalities are not affected? It may not be a good practice to have a |
gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java
Outdated
Show resolved
Hide resolved
Added usage of the EndpointContext in the ClientContext to determine the endpoint. |
public abstract String clientSettingsEndpoint(); | ||
|
||
@Nullable | ||
public abstract String transportChannelEndpoint(); |
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.
Will we ever need to pass in transportChannelEndpoint
?
I see that there is no getters for the endpoint set to transport channel, maybe we want to expose one? So that we can set all the info to EndpointContext
?
It's also OK to leave transportChannelEndpoint
out of EndpointContext
for now, if that's the case, then it's better to remove this field at this moment.
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.
Will we ever need to pass in transportChannelEndpoint?
I believe so to keep with the existing behavior where transportChannelEndpoint overrides the clientSettingsEndpoint if set. Right now, the ClientContext doesn't set it inside the EndpointContext but the code to use it exists here:
sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java
Lines 233 to 235 in da6607b
if (transportChannelProvider.needsEndpoint()) { | |
transportChannelProvider = transportChannelProvider.withEndpoint(endpoint); | |
} |
The reason I haven't added it in yet is because I most likely will need to expose an additional getEndpoint()
method to the TransportChannelProvider: https://github.com/googleapis/sdk-platform-java/blob/da6607b17130ab045640618d505fda915ddb8e49/gax-java/gax/src/main/java/com/google/api/gax/rpc/TransportChannelProvider.java (Note: grpc and httpjson's transportchannel already have a getEndpoint()
method). I was planning on doing this on the next PR as this one only introduces the EndpointContext.
I see your point about including it then, so I'll drop the transportChannelEndpoint values from this PR and add it in the next PR.
return endpoint; | ||
} | ||
|
||
public String resolveEndpoint() throws IOException { |
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.
Can we try to resolve the endpoint in the build()
method and just expose a getter for the resolved endpoint?
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.
I'm using Autovalue's Builder which I believe generates the build()
implementation for us. I could create my own Builder class if that's preferable.
Edit: Actually, I think I see a section that I can configure. I'll try that.
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.
@AutoValue | ||
public abstract class EndpointContext { | ||
@Nullable | ||
public abstract String clientSettingsEndpoint(); |
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.
I don't think this can be null as it is coming from StubSettings.endpoint?
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.
I believe for most non-modified GAPICs, this is the case. It should be set via the {Service}StubSettings like: https://github.com/googleapis/google-cloud-java/blob/6227504db7c511b379cad2d67960d63f36eed39b/java-grafeas/src/main/java/io/grafeas/v1/stub/GrafeasStubSettings.java#L635 with a non-null value (as the generator will fail if it can't find it).
I'm accounting for the Grafeas edge case: https://github.com/googleapis/google-cloud-java/blob/6227504db7c511b379cad2d67960d63f36eed39b/java-grafeas/src/main/java/io/grafeas/v1/stub/GrafeasStubSettings.java#L403-L405 which has post-processed changes.
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.
I see, this is good for now then. Let's keep this in mind and come back for it later if we have time. I'm not sure why we removed the default endpoint for grafeas, and how does grafeas create channels without a default endpoint. But ideally this should not be nullable, otherwise gRPC channel creation could fail.
gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java
Outdated
Show resolved
Hide resolved
1f56ada
to
a6eeda8
Compare
Removed the methods and seeing CLIRR check failures:
This was due to the old gax version being cached in gh actions. Cleared the cache and re-ran. |
gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java
Outdated
Show resolved
Hide resolved
[gapic-generator-java-root] SonarCloud Quality Gate failed. 0 Bugs 0.0% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
[java_showcase_integration_tests] SonarCloud Quality Gate failed. 0 Bugs 74.4% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
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.
LGTM. Before merging, can you please add more info to the description to explain the purpose of EndpointContext
?
I would reword the title as well, I think this is more like a refactor
instead of feat
. Because we added a new class to encapsulate existing features but didn't introduce any new features.
Added. |
Thank you for opening a Pull Request! For general contributing guidelines, please refer to contributing guide
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Description
EndpointContext is a new class created to resolve the correct endpoint. Client library users are able customize the endpoint in various ways and this class is the source of truth to resolve all the customizations.
Currently supports:
The logic to resolve the endpoint was moved from the ClientContext into the EndpointContext. In this PR, no additional logic was added.