-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat: GA Gax HttpJson #1264
Conversation
Kudos, SonarCloud Quality Gate passed! |
@@ -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.") |
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.
Issue to better support extraHeaders: #1752
Will be done in the future.
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.
If this BetaApi annotation is removed in gax-httpjson, should it also be removed in gax and gax-grpc?
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.
If the base class/interface is still BetaApi
, we don't have to remove them, I would only remove BetaApi
for httpjson specific classes.
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.
If the base class/interface is still
BetaApi
, we don't have to remove them, I would only removeBetaApi
for httpjson specific classes.
Just to clarify what you mean by httpjson specific classes. Do you mean:
- Remove annotation for classes/ methods that exist ONLY in the httpjson module and no other gax module (
withExtraHeaders
would keep the BetaApi annotation) - Remove annotation only from the httpjson module (i.e. Remove in httpjson but keep in gax and gax-grpc)
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.
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).
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.
Gotcha. I'll check the files again and update.
@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. |
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. |
Things left with BetaApi annotation:
Note:
Weird behaviors:
|
I've updated the current for gax-httpjson to be |
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 Lawrence! LGTM.
coverage-report/pom.xml
Outdated
@@ -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} --> |
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 you remove {x-version-update:gax-httpjson:current}
altogether in favor of version.gax
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.
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)?
HttpJsonCallContext's withExtraHeaders is marked as BetaApi. Is this intentional (I'm not saying you should remove it).
|
This is intentional. The context is here: #1264 (comment). |
[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!
|
[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!
|
[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!
|
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.