Refactoring to allow multiple transports in GAPIC clients #2156

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.

Sign up for GitHub

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

Jump to bottom

Merged

garrettjonesgoogle merged 4 commits into googleapis:master from garrettjonesgoogle:master

Jul 20, 2017

Member

garrettjonesgoogle commented Jun 16, 2017

No description provided.

googlebot added the cla: yes label

garrettjonesgoogle force-pushed the master branch 2 times, most recently from 14d346c to 8b036cb Compare

July 6, 2017 19:49


          [WIP] Refactoring to allow multiple transports

3c848bf

garrettjonesgoogle force-pushed the master branch from 8b036cb to 3c848bf Compare

July 13, 2017 19:17

garrettjonesgoogle assigned shinfan, pongad and michaelbausor

Member Author

garrettjonesgoogle commented Jul 13, 2017

This is ready to review; it is only WIP because gax-java won't be released until this PR is approved.

garrettjonesgoogle requested review from michaelbausor, pongad and shinfan

July 17, 2017 16:06

Member Author

garrettjonesgoogle commented Jul 17, 2017

gax-java and toolkit are approved, PTAL here

Contributor

michaelbausor commented Jul 17, 2017

LGTM, others should approve also

shinfan reviewed

View reviewed changes

Contributor

shinfan left a comment

Overall looks good to me. Just a few comments & questions

google-cloud-dlp/src/main/java/com/google/cloud/dlp/v2beta1/DlpServiceSettings.java Outdated

    
                }

                /**

                 * Returns a builder for this class with recommened defaults for API methods, and the given

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-dlp/src/main/java/com/google/cloud/dlp/v2beta1/DlpServiceSettings.java

    
                    settingsBuilder =

                    ImmutableMap.Builder<String, RetrySettings> definitions = ImmutableMap.builder();

                    RetrySettings settings = null;

                    settings =

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-dlp/src/main/java/com/google/cloud/dlp/v2beta1/stub/DlpServiceStub.java

    
              // AUTO-GENERATED DOCUMENTATION AND CLASS

              @Generated("by GAPIC v0.0.5")

              @BetaApi

              public abstract class DlpServiceStub implements BackgroundResource {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-dlp/src/main/java/com/google/cloud/dlp/v2beta1/stub/GrpcDlpServiceStub.java Outdated

    
                /**

                 * Initiates an orderly shutdown in which preexisting calls continue but new calls are immediately

                 * cancelled.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-dlp/src/main/java/com/google/cloud/dlp/v2beta1/stub/GrpcDlpServiceStub.java

    
                @Override

                public void shutdown() {

                  backgroundResources.shutdown();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-dlp/src/main/java/com/google/cloud/dlp/v2beta1/stub/GrpcDlpServiceStub.java

    
              // AUTO-GENERATED DOCUMENTATION AND CLASS

              @Generated("by GAPIC v0.0.5")

              @BetaApi

              public class GrpcDlpServiceStub extends DlpServiceStub {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-dlp/src/main/java/com/google/cloud/dlp/v2beta1/stub/GrpcDlpServiceStub.java

    
                                    ListRootCategoriesRequest.getDefaultInstance()),

                                io.grpc.protobuf.ProtoUtils.marshaller(

                                    ListRootCategoriesResponse.getDefaultInstance())));

                private final BackgroundResource backgroundResources;

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-dlp/src/main/java/com/google/cloud/dlp/v2beta1/stub/GrpcDlpServiceStub.java Outdated

    
                /**

                 * Constructs an instance of GrpcDlpServiceStub, using the given settings. This is protected so

                 * that it easy to make a subclass, but otherwise, the static factory methods should be preferred.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Contributor

pongad commented Jul 19, 2017 •

edited

Loading

The code LGTM, assuming @shinfan 's concerns are addressed.

I'm not sure if this could cause a performance hit because we're wrapping more stuff. Though I'm not sure if we should block on this.


          Addressing PR feedback

a32333a

Member Author

garrettjonesgoogle commented Jul 19, 2017

Addressed Shin's comments, PTAL @shinfan

Member Author

garrettjonesgoogle commented Jul 19, 2017

@pongad I don't think the extra stack frame matters. I think what does matter is that gax was refactored a bit to make this happen, so allocation characteristics may have changed at that level. Like you say, I don't think possible performance changes should impact this PR; however, I think it might be worth doing a perf test before we do a release of google-cloud-java.

shinfan approved these changes

View reviewed changes

Contributor

shinfan left a comment

LGTM

Contributor

pongad commented Jul 19, 2017

@garrettjonesgoogle I agree. That's what I meant; I just didn't think of this when looking at the gax PR. I'll work on measurement when I get cycles.

garrettjonesgoogle added 2 commits

July 20, 2017 11:27


          Merge branch 'master' of https://github.com/GoogleCloudPlatform/gclou…

b9134dd

…d-java


          Using released gax versions

d5575d8

garrettjonesgoogle changed the title ~~[WIP] Refactoring to allow multiple transports~~ Refactoring to allow multiple transports in GAPIC clients

coveralls commented Jul 20, 2017

Changes Unknown when pulling d5575d8 on garrettjonesgoogle:master into ** on GoogleCloudPlatform:master**.

garrettjonesgoogle merged commit 0e4ed46 into googleapis:master

garrettjonesgoogle mentioned this pull request

Adding @BetaApi to particular exception constructors #2286

Merged

theacodes unassigned shinfan, pongad and michaelbausor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels