Update google api header by shinfan · Pull Request #179 · googleapis/gax-java

shinfan · 2017-01-23T19:48:03Z

Now the gapic header has the following format:
Gapic only client: gl-java/{java_version} gapic/0.1.0 gax/0.0.27 grpc/1.0.1
Hand-written client: gl-java/{java_version} gccl/0.0.0 gapic/0.1.0 gax/0.0.27 grpc/1.0.1

The hand-written layer can override the top level token 'gapic/0.1.0' by setting the client name/version.

Add unit test

codecov-io · 2017-01-23T20:06:12Z

Codecov Report

Merging #179 into master will increase coverage by 0.34%.

@@ Coverage Diff @@## master #179 +/- ##
============================================
+ Coverage 70.04% 70.38% +0.34% - Complexity 460 463 +3 
============================================
Files 64 64 Lines 2420 2418 -2 Branches 260 262 +2 ============================================
+ Hits 1695 1702 +7 + Misses 631 620 -11 - Partials 94 96 +2

Impacted Files	Coverage Δ	Complexity Δ
...gle/api/gax/grpc/InstantiatingChannelProvider.java	`53.16% <65.21%> (+9.95%)`	`10 <4> (+3)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0f5b1b...69aa90a. Read the comment docs.

Sign in to view

 */
 public final class InstantiatingChannelProvider implements ChannelProvider {
 private static final String DEFAULT_GAX_VERSION = "0.0.27";
+ private static final String DEFAULT_GRPC_VERSION = "1.0.1";


Sign in to view

- "%s/%s %s/%s gax/%s java/%s",
- clientLibName,
- clientLibVersion,
+ "%s/%s gax/%s grpc/%s java/%s",


Sign in to view

 // Default names and versions of the client and the service generator.
 private static final String DEFAULT_GENERATOR_NAME = "gapic";
- private static final String DEFAULT_CLIENT_LIB_NAME = "gax";
+ private static final String DEFAULT_CLIENT_LIB_NAME = "default";


Sign in to view

+ String.format(
+ "gapic/0.1.0 gax/0.0.27 grpc/1.0.1 java/%s default/0.1.0",
+ Runtime.class.getPackage().getImplementationVersion());
+ assertEquals(provider.serviceHeader(), expectedHeader);


bjwatson · 2017-01-26T21:17:17Z

I'll review this soon. The design doc is not fully baked, since there are still a few unresolved concerns.

- Add unit test

shinfan · 2017-02-08T23:38:52Z

@bjwatson Friendly ping~

bjwatson · 2017-02-08T23:40:44Z

Thanks for the updates @shinfan. I'm reviewing now.

Sign in to view

 */
 public final class InstantiatingChannelProvider implements ChannelProvider {
 private static final String DEFAULT_GAX_VERSION = "UNKNOWN";
+ private static final String DEFAULT_GRPC_VERSION = "1.0.1";


Sign in to view

 // Default names and versions of the client and the service generator.
 private static final String DEFAULT_GENERATOR_NAME = "gapic";
- private static final String DEFAULT_CLIENT_LIB_NAME = "gax";
+ private static final String DEFAULT_CLIENT_LIB_NAME = "default";


bjwatson

Mostly looks good. We might move gl-java to the beginning of the list (see the email thread today). I just have a few requested changes, and want to confirm that corresponding toolkit changes are also needed.

Sign in to view

@@ -159,6 +150,22 @@ public boolean shouldAutoClose() {
 }

 @VisibleForTesting


Sign in to view

 @RunWith(JUnit4.class)
 public class InstantiatingChannelProviderTest {

+ @Test


Sign in to view

- this.serviceGeneratorName = name;
- this.serviceGeneratorVersion = version;
+ /** Sets the generator name and version for the GRPC custom header. */
+ public Builder setClientHeader(String name, String version) {


Sign in to view

+ String grpcVersion = ManagedChannel.class.getPackage().getImplementationVersion();
+ if (grpcVersion == null) {
+ grpcVersion = DEFAULT_GRPC_VERSION;
+ }


Sign in to view

+ grpcVersion = DEFAULT_GRPC_VERSION;
+ }
+
+ String javaVersion = Runtime.class.getPackage().getImplementationVersion();


shinfan · 2017-02-09T01:19:03Z

@bjwatson PTAL

bjwatson · 2017-02-09T15:42:32Z

LGTM

Sign in to view

- String clientLibName,
- String clientLibVersion,
- String serviceGeneratorName,
- String serviceGeneratorVersion) {


bjwatson · 2017-02-09T18:51:45Z

Because the gccl version implies the gapic version for all source delivery languages. When you switch Datastore to use GAPIC, then you'll know the gccl version as of which that happened. (Of course we'll need to keep track of this somehow) You can add the gapic version to the header if you really want to, but be aware that you're adding some server overhead to every call without providing any additional information. On Feb 9, 2017 10:13 AM, "garrettjonesgoogle" <notifications@github.com> wrote: *@garrettjonesgoogle* commented on this pull request. ------------------------------ In src/main/java/com/google/api/gax/grpc/InstantiatingChannelProvider.java <#179 (review)> :

private InstantiatingChannelProvider( ExecutorProvider executorProvider, CredentialsProvider credentialsProvider, String serviceAddress, int port, - String clientLibName, - String clientLibVersion, - String serviceGeneratorName, - String serviceGeneratorVersion) { I still don't understand why we aren't retaining both the gapic version and the gccl version when both are relevant. There is a particular scenario I want recorded: Datastore as of now uses the legacy Google API Client stack, thus it would have gccl and not gapic; but, when we switch it to using a GAPIC client, then I want gccl and gapic to both appear. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#179 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAcyy2Nd2KbdwteTqxXErxGBw9CA78GSks5ra1ddgaJpZM4Lrdvu> .

garrettjonesgoogle · 2017-02-09T19:30:33Z

Because the gccl version implies the gapic version for all source delivery languages. When you switch Datastore to use GAPIC, then you'll know the gccl version as of which that happened.

(Of course we'll need to keep track
of this somehow)

That's the part that concerns me. I don't want to track this.

You can add the gapic version to the header if you really want to, but be aware that you're adding some server overhead to every call without providing any additional information.

But it actually is additional information - information that is otherwise a chore to uncover, because it requires digging up old source code revisions to find it out. Think of this scenario: there could be cases where we regenerate some GAPIC clients but not others, and thus for a particular gccl version, the gapic version depends on the API.

garrettjonesgoogle · 2017-02-09T19:31:47Z

Oh and I forgot to mention the scenario of an API starting with GAPIC-only, then converting to having a handwritten layer. In that case, the gapic version would disappear and there would be no continuity.

shinfan · 2017-02-09T20:14:23Z

@garrettjonesgoogle PTAL

garrettjonesgoogle · 2017-02-09T20:19:12Z

LGTM

Sign in to view

- // Default names and versions of the client and the service generator.
+ // Default names and versions of the service generator.
 private static final String DEFAULT_GENERATOR_NAME = "gapic";
- private static final String DEFAULT_CLIENT_LIB_NAME = "gax";


shinfan assigned bjwatson, pongad and garrettjonesgoogle Jan 23, 2017

shinfan mentioned this pull request Jan 23, 2017
bump version to 0.0.28 for release #178
Closed

garrettjonesgoogle reviewed Jan 24, 2017
View reviewed changes

bjwatson self-requested a review January 26, 2017 21:15

shinfan force-pushed the master branch from b9b5ef0 to 2d91468Compare February 2, 2017 19:01

shinfan added 3 commits February 8, 2017 15:09

Update google api header
1e99b0f
- Add unit test

Merge branch 'master' of github.com:googleapis/gax-java
4c9ba48

Update PR
d398c97

shinfan force-pushed the master branch from 2d91468 to d398c97Compare February 8, 2017 23:32

Fix typos
5743b5b

bjwatson reviewed Feb 8, 2017
View reviewed changes

bjwatson suggested changes Feb 9, 2017
View reviewed changes

shinfan added 3 commits February 8, 2017 16:59

Update PR feedback
0e8cb81

Update more PR feedback
affaa05

Move gl-java to front
ce18246

bjwatson approved these changes Feb 9, 2017
View reviewed changes

garrettjonesgoogle reviewed Feb 9, 2017
View reviewed changes

Comment threadsrc/main/java/com/google/api/gax/grpc/InstantiatingChannelProvider.java
String clientLibName,
String clientLibVersion,
String serviceGeneratorName,
String serviceGeneratorVersion) {
This comment was marked as spam.
Sign in to view

garrettjonesgoogle mentioned this pull request Feb 9, 2017
bump to 0.0.29 for release #200
Merged

Put both gapic and gccl in
9b84205

shinfan added 2 commits February 9, 2017 12:10

Update
93fea68

Minor renaming
69aa90a

shinfan merged commit 6a4c211 into googleapis:masterFeb 9, 2017

bjwatson reviewed Feb 10, 2017
View reviewed changes

garrettjonesgoogle mentioned this pull request Feb 10, 2017
Format for x-goog-api-client needs updating #86
Closed

		@@ -159,6 +150,22 @@ public boolean shouldAutoClose() {
		}

		@VisibleForTesting

Uh oh!

Conversation

shinfan commented Jan 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jan 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

bjwatson commented Jan 26, 2017

Uh oh!

shinfan commented Feb 8, 2017

Uh oh!

bjwatson commented Feb 8, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

bjwatson left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

shinfan commented Feb 9, 2017

Uh oh!

bjwatson commented Feb 9, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

bjwatson commented Feb 9, 2017 via email

Uh oh!

garrettjonesgoogle commented Feb 9, 2017

Uh oh!

garrettjonesgoogle commented Feb 9, 2017

Uh oh!

shinfan commented Feb 9, 2017

Uh oh!

garrettjonesgoogle commented Feb 9, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

shinfan commented Jan 23, 2017•
edited
Loading

codecov-io commented Jan 23, 2017•
edited
Loading