Skip to content
This repository was archived by the owner on Sep 26, 2023. It is now read-only.

Use GoogleDefaultChannelBuilder #627

Merged
andreamlin merged 2 commits into from
Jan 23, 2019
Merged

Use GoogleDefaultChannelBuilder #627

andreamlin merged 2 commits into from
Jan 23, 2019

Conversation

@andreamlin
Copy link
Contributor

@andreamlin andreamlin commented Dec 17, 2018

Resolves #626.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 17, 2018
@andreamlin
Copy link
Contributor Author

andreamlin commented Dec 17, 2018

Still need to test with google-cloud-java.

@codecov-io
Copy link

codecov-io commented Dec 17, 2018 •
edited by codecov bot
Loading

Codecov Report

Merging #627 into master will increase coverage by 0.34%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #627      +/-   ##
============================================
+ Coverage     74.94%   75.29%   +0.34%     
  Complexity      935      935              
============================================
  Files           177      177              
  Lines          4099     4080      -19     
  Branches        328      328              
============================================
  Hits           3072     3072              
+ Misses          874      855      -19     
  Partials        153      153
Impacted Files Coverage Δ Complexity Δ
...api/gax/grpc/InstantiatingGrpcChannelProvider.java 74.45% <85.71%> (+9.06%) 26 <0> (ø) ⬇️

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 1d492dc...846420c. Read the comment docs.

@andreamlin andreamlin requested a review from yihanzhen December 17, 2018 21:42
@andreamlin
Copy link
Contributor Author

andreamlin commented Dec 17, 2018

PTAL

@andreamlin
Copy link
Contributor Author

andreamlin commented Dec 17, 2018 •
edited
Loading

See b/120606286#comment16

yihanzhen
yihanzhen approved these changes Dec 18, 2018
View reviewed changes
Copy link
Contributor

@yihanzhen yihanzhen left a comment

Choose a reason for hiding this comment

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

lgtm with two trivial comments.

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Outdated
builder.maxInboundMetadataSize(maxInboundMetadataSize);
}

builder

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Outdated
int port = Integer.parseInt(endpoint.substring(colon + 1));
String serviceAddress = endpoint.substring(0, colon);

// TODO(hzyi): Change to ManagedChannelBuilder directly when

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@ejona86
Copy link

ejona86 commented Dec 18, 2018

Swapping everyone over to GoogleDefaultChannelBuilder would break users in ClassLoading environments because it will leak a thread and thus the class loader. You should really wait for grpc/grpc-java#4755 to be fixed and released.

ejona86
ejona86 reviewed Dec 18, 2018
View reviewed changes
Copy link

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

It LGTM, but as I mentioned, I don't think GoogleDefaultChannelBuilder is ready for all users to begin using it. That needs to be resolved on the gRPC-side.

@andreamlin andreamlin added do not merge Indicates a pull request not ready for merge, due to either quality or timing. blocked labels Dec 18, 2018
@mziccard
Copy link

mziccard commented Jan 23, 2019

@andreamlin seems that the issue @ejona86 mentioned has recently been fixed (grpc/grpc-java#5210). Any chance this can be revamped now?

@andreamlin
Copy link
Contributor Author

andreamlin commented Jan 23, 2019

@mziccard thanks for the heads up.

I'll wait for @jiangtaoli2016 to confirm.

@andreamlin andreamlin merged commit 6c8cd40 into googleapis:master Jan 23, 2019
@andreamlin andreamlin deleted the GoogleDefaultChannelBuilder branch January 23, 2019 21:18
@ejona86
Copy link

ejona86 commented Jan 23, 2019

Note that the issue is only fixed on master, not the grpc-java 1.18.x release. However, SRV is disabled by default so you shouldn't hit that issue unless you are explicitly passing a flag to grpc to enable SRV/ALTS.

@apolcyn apolcyn mentioned this pull request Jan 29, 2019
Closed
@ejona86
Copy link

ejona86 commented Jan 31, 2019 •
edited
Loading

This should probably be reverted because of grpc/grpc-java#5303 . If you let users specify the CallCredentials then that will become a NOOP with this change.

Filed #649 . Note that a wholesale revert wouldn't be as good as just replacing the GoogleDefaultChannelBuilder with ManagedChannelBuilder.

@ejona86 ejona86 mentioned this pull request Jan 31, 2019
Closed
This was referenced Jan 31, 2019
Closed
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@ejona86 ejona86 ejona86 left review comments

@yihanzhen yihanzhen yihanzhen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

blocked cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@andreamlin @codecov-io @ejona86 @mziccard @yihanzhen @googlebot