Skip to content
This repository was archived by the owner on Jan 19, 2022. It is now read-only.

Conversation

@ST-DDT
Copy link
Contributor

@ST-DDT ST-DDT commented Dec 17, 2019

Improves compatibility with libraries that provide additional grpc NameResolvers

See also: grpc/grpc-java#6499

yidongnan/grpc-spring-boot-starter#268
yidongnan/grpc-spring-boot-starter#304

Improves compatibility with libraries that provide additional NameResolvers

See also: grpc/grpc-java#6499
@codecov
Copy link

codecov bot commented Dec 17, 2019

Codecov Report

Merging #2085 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2085   +/-   ##
=========================================
  Coverage     72.48%   72.48%           
  Complexity     1886     1886           
=========================================
  Files           244      244           
  Lines          6966     6966           
  Branches        715      715           
=========================================
  Hits           5049     5049           
  Misses         1586     1586           
  Partials        331      331
Flag Coverage Δ Complexity Δ
#integration ? ?
#unittests 72.48% <100%> (ø) 1886 <1> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
...igure/firestore/GcpFirestoreAutoConfiguration.java 91.17% <100%> (ø) 4 <0> (ø) ⬇️
...ure/pubsub/GcpPubSubEmulatorAutoConfiguration.java 100% <100%> (ø) 2 <0> (ø) ⬇️
...igure/trace/StackdriverTraceAutoConfiguration.java 69.49% <100%> (ø) 10 <1> (ø) ⬇️

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 a8217d6...29845c8. Read the comment docs.

Copy link
Contributor

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

I think you missed two other places in GcpPubSubEmulatorAutoConfiguration and GcpFirestoreAutoConfiguration.

In tests I also see us as configuring the ManagedChannel using forAddress instead of forTarget.
Which is more appropriate?

Copy link
Contributor

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

Thanks! Once the tests pass, we can merge.
We might also want to apply this to the 1.2.x branch.

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Dec 17, 2019

I think you missed two other places in GcpPubSubEmulatorAutoConfiguration and GcpFirestoreAutoConfiguration.

Those parts are configured via properties so I didn't include them in the first run.
(I hardcoded them at the place they are used, so it might no longer be possible to configure a different scheme via the config, but I don't know whether this was possible in the first place)

In tests I also see us as configuring the ManagedChannel using forAddress instead of forTarget.
Which is more appropriate?

forAddress is fine for tests and static addresses, but it won't get updated and IIRC doesn't support load balancing.
forTarget also supports dynamic addresses such as DNS and also supports resolution of multiple matches and thus allows client side load balancing.

I changed the test to match the other locations.

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Dec 17, 2019

We might also want to apply this to the 1.2.x branch.

(Not sure what this implicates)
Should I create a second PR which adds it to that branch
or will you do that yourself and that is just an information for me?

@meltsufin
Copy link
Contributor

@ST-DDT I can take care of the merge into 1.2.x.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants