Skip to content
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: GA Gax HttpJson #1264

Merged
merged 13 commits into from
Jun 20, 2023
Merged

feat: GA Gax HttpJson #1264

merged 13 commits into from
Jun 20, 2023

Conversation

lqiu96
Copy link
Contributor

@lqiu96 lqiu96 commented Jan 20, 2023

We will begin the process of trying to GA Gax's HttpJson module. @BetaApi will slowly be removed where it makes sense as we have observed stability in this module.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Jun 8, 2023
@@ -326,7 +326,6 @@ public Duration getStreamIdleTimeout() {
return streamIdleTimeout;
}

@BetaApi("The surface for extra headers is not stable yet and may change in the future.")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue to better support extraHeaders: #1752

Will be done in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this BetaApi annotation is removed in gax-httpjson, should it also be removed in gax and gax-grpc?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the base class/interface is still BetaApi, we don't have to remove them, I would only remove BetaApi for httpjson specific classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the base class/interface is still BetaApi, we don't have to remove them, I would only remove BetaApi for httpjson specific classes.

Just to clarify what you mean by httpjson specific classes. Do you mean:

  1. Remove annotation for classes/ methods that exist ONLY in the httpjson module and no other gax module (withExtraHeaders would keep the BetaApi annotation)
  2. Remove annotation only from the httpjson module (i.e. Remove in httpjson but keep in gax and gax-grpc)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 yes, 2 no. In addition, if a class does not extend from classes in other modules, we should remove BetaApi from it unless there is a reason not to(e.g. maybe the feature in grpc is also still in Beta).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. I'll check the files again and update.

@lqiu96 lqiu96 added the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 8, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 8, 2023
@lqiu96 lqiu96 marked this pull request as ready for review June 8, 2023 20:12
@lqiu96 lqiu96 requested a review from a team as a code owner June 8, 2023 20:12
@lqiu96 lqiu96 requested review from suztomo, blakeli0 and ddixit14 June 8, 2023 20:18
@lqiu96
Copy link
Contributor Author

lqiu96 commented Jun 8, 2023

@blakeli0 @suztomo @ddixit14 I'm looking to begin the process of GAing Gax-HttpJson. I want to see your opinions of what version I should set for this module's GA: v1.0.0 or v2.30.0 (I believe v2.30.0 should be the next version of gax & gax-grpc).

Immediately bumping to v2.30.0 would keep all the gax versions in sync, but may be confusing to customers who see the bump from v0.114.1 -> v2.30.0.

@suztomo
Copy link
Member

suztomo commented Jun 9, 2023

I'm for "v2.30.0 (should match the next version of gax & gax-grpc)". The consistent version gives clarity in error messages when a dependency conflict happens.

@lqiu96
Copy link
Contributor Author

lqiu96 commented Jun 9, 2023

Things left with BetaApi annotation:

  1. HttpJsonCallContext: withExtraHeaders and getExtraHeaders - HttpJsonCallContext extends from ApiCallContext and the methods override from there. The parent methods still have BetaApi annotation

Note:

  1. HttpJsonOperationSnapshotCallable extends from UnaryCallable, but UnaryCallable has no BetaApi annotation (also oddly enough has both BetaApi and Internal annotations)
  2. HttpJsonStatusCode extends from StatusCode, but StatusCode has no BetaApi annotation

Weird behaviors:

  1. InstantiatingHttpJsonChannelProvider extends from TransportChannelProvider. TransportChannelProvider has two BetaApi annotations in needsCredentials and withCredentials, but InstantiatingHttpJsonChannelProvider's overriden methods don't have BetaApi annotations

@lqiu96
Copy link
Contributor Author

lqiu96 commented Jun 9, 2023

I'm for "v2.30.0 (should match the next version of gax & gax-grpc)". The consistent version gives clarity in error messages when a dependency conflict happens.

I've updated the current for gax-httpjson to be 2.29.1-SNAPSHOT and I believe with this feat: PR, it'll mark it as 2.30.0. If anyone has any concerns, let me know.

@lqiu96
Copy link
Contributor Author

lqiu96 commented Jun 20, 2023

@suztomo @blakeli0 @ddixit14 Would you all be able to review this PR? If there are any concerns, I can target a different cycle to unblock this release train.

Copy link
Collaborator

@blakeli0 blakeli0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Lawrence! LGTM.

@@ -41,7 +41,7 @@
<dependency>
<groupId>com.google.api</groupId>
<artifactId>gax-httpjson</artifactId>
<version>0.114.1-SNAPSHOT</version> <!-- {x-version-update:gax-httpjson:current} -->
<version>2.29.1-SNAPSHOT</version> <!-- {x-version-update:gax-httpjson:current} -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove {x-version-update:gax-httpjson:current} altogether in favor of version.gax

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good idea! Updated in this file as well as the gax-bom. I noticed that we still have gax-grpc. Shall I remove this that reference as well (or maybe in a different PR)?

@suztomo
Copy link
Member

suztomo commented Jun 20, 2023

HttpJsonCallContext's withExtraHeaders is marked as BetaApi. Is this intentional (I'm not saying you should remove it).

~/sdk-platform-java/gax-java/gax-httpjson $ grep -ilr -A 2 '@BetaApi' .                    git[branch:main-gax_httpjson_GA]
./src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java
[email protected] ~/sdk-platform-java/gax-java/gax-httpjson
~/sdk-platform-java/gax-java/gax-httpjson $ grep -ir -A 2 '@BetaApi' .                     git[branch:main-gax_httpjson_GA]
./src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java:  @BetaApi("The surface for extra headers is not stable yet and may change in the future.")
./src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java-  @Override
./src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java-  public ApiCallContext withExtraHeaders(Map<String, List<String>> extraHeaders) {
--
./src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java:  @BetaApi("The surface for extra headers is not stable yet and may change in the future.")
./src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java-  @Override
./src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java-  public Map<String, List<String>> getExtraHeaders() {

@lqiu96
Copy link
Contributor Author

lqiu96 commented Jun 20, 2023

HttpJsonCallContext's withExtraHeaders is marked as BetaApi. Is this intentional (I'm not saying you should remove it).

~/sdk-platform-java/gax-java/gax-httpjson $ grep -ilr -A 2 '@BetaApi' .                    git[branch:main-gax_httpjson_GA]
./src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java
[email protected] ~/sdk-platform-java/gax-java/gax-httpjson
~/sdk-platform-java/gax-java/gax-httpjson $ grep -ir -A 2 '@BetaApi' .                     git[branch:main-gax_httpjson_GA]
./src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java:  @BetaApi("The surface for extra headers is not stable yet and may change in the future.")
./src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java-  @Override
./src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java-  public ApiCallContext withExtraHeaders(Map<String, List<String>> extraHeaders) {
--
./src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java:  @BetaApi("The surface for extra headers is not stable yet and may change in the future.")
./src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java-  @Override
./src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java-  public Map<String, List<String>> getExtraHeaders() {

This is intentional. The context is here: #1264 (comment).

@sonarqubecloud
Copy link

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sonarqubecloud
Copy link

[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sonarqubecloud
Copy link

[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@lqiu96 lqiu96 merged commit 9f15fea into main Jun 20, 2023
@lqiu96 lqiu96 deleted the main-gax_httpjson_GA branch June 20, 2023 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants