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.

src/main/java/com/google/api/gax/grpc/InstantiatingChannelProvider.java

 */
 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";


src/main/java/com/google/api/gax/grpc/InstantiatingChannelProvider.java

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


src/main/java/com/google/api/gax/grpc/InstantiatingChannelProvider.java

    // 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";


src/test/java/com/google/api/gax/grpc/InstantiatingChannelProviderTest.java

+        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.

src/main/java/com/google/api/gax/grpc/InstantiatingChannelProvider.java

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


src/main/java/com/google/api/gax/grpc/InstantiatingChannelProvider.java

    // 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.

src/main/java/com/google/api/gax/grpc/InstantiatingChannelProvider.java

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

  @VisibleForTesting


src/test/java/com/google/api/gax/grpc/InstantiatingChannelProviderTest.java

 @RunWith(JUnit4.class)
 public class InstantiatingChannelProviderTest {

+  @Test


src/main/java/com/google/api/gax/grpc/InstantiatingChannelProvider.java

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


src/main/java/com/google/api/gax/grpc/InstantiatingChannelProvider.java

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


src/main/java/com/google/api/gax/grpc/InstantiatingChannelProvider.java

+      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

src/main/java/com/google/api/gax/grpc/InstantiatingChannelProvider.java

-      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

src/main/java/com/google/api/gax/grpc/InstantiatingChannelProvider.java

-    // 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 2d91468 Compare 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 d398c97 Compare 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

src/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:master Feb 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

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!

This comment was marked as spam.

shinfan commented Jan 23, 2017 •

edited

Loading

codecov-io commented Jan 23, 2017 •

edited

Loading