Skip to content
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.

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

Plug Akka Discovery into the grpc-java NameResolver #811

Merged
merged 9 commits into from Feb 25, 2020

Conversation

@raboof
Copy link
Member

raboof commented Feb 6, 2020

Instead of performing discovery 'outside' of the Channel.

This would allow configuring client-side loadbalancing, #809, but I have
not yet tested that that goal is actually achieved.

Before un-drafting this generally needs some more testing whether it
does the right thing, especially re-resolving names in case of updates
etc. It may also mean we can simplify/remove the recreation code in
ClientState.scala, but that also still needs to be verified.

@raboof raboof force-pushed the plugDiscoveryIntoNameResolver branch from 7f221ff to 2576c5d Feb 7, 2020
@raboof raboof added this to the 0.8.0 milestone Feb 19, 2020
@raboof raboof mentioned this pull request Feb 20, 2020
raboof added 3 commits Feb 6, 2020
Instead of performing discovery 'outside' of the Channel.

This would allow configuring client-side loadbalancing, #809, but I have
not yet tested that that goal is actually achieved.

Before un-drafting this generally needs some more testing whether it
does the right thing, especially re-resolving names in case of updates
etc. It may also mean we can simplify/remove the recreation code in
ClientState.scala, but that also still needs to be verified.
@raboof raboof force-pushed the plugDiscoveryIntoNameResolver branch from 2576c5d to 181356f Feb 20, 2020
@raboof

This comment has been minimized.

Copy link
Member Author

raboof commented Feb 20, 2020

This is now ready for review, though I'd like to double-check if it ever makes sense to explicitly pass in DnsNameResolverProvider

@raboof raboof marked this pull request as ready for review Feb 20, 2020
@raboof raboof requested a review from ignasi35 Feb 21, 2020
@raboof raboof added this to Ready for review in Akka streaming Feb 21, 2020
// val settings = GrpcClientSettings.usingServiceDiscovery("testService")
//
// val channel = NettyClientUtils.createChannel(settings)
// }
}

This comment has been minimized.

Copy link
@ignasi35

ignasi35 Feb 24, 2020

Member

With the changes in NettyClientUtils the invocation to val channel = builder.build() is not longer within a try{}catch (or a future.map{} as it used to be). Therefore an invocation to createChannel() will either return a successful Future or throw a runtime exception but never a Future.failed?

This comment has been minimized.

Copy link
@raboof

raboof Feb 24, 2020

Author Member

I didn't mean for that code to ever throw runtime exceptions. I kept the Future in the API for now to not change too much at once.

}

def lookup(listener: Listener): Unit = {
discovery.lookup(Lookup(authority, portName, protocol), resolveTimeout).onComplete {

This comment has been minimized.

Copy link
@ignasi35

ignasi35 Feb 24, 2020

Member

I think the name of the authority field is confusing. Here, for example, it's used as a service name on a service discovery lookup, but on line 28 it's used as an actual authority. Instead, we may need to pass a serviceName for the lookups and the value of GrpcSettings#overrideAuthority (or serviceName if unset).

This change here is related to the line .forTarget("//" + settings.serviceName) above.

This comment has been minimized.

Copy link
@raboof

raboof Feb 24, 2020

Author Member

I agree this is somewhat confusing, but I'm following the lead of the upstream impl here (https://github.com/grpc/grpc-java/blob/master/core/src/main/java/io/grpc/internal/DnsNameResolver.java#L174)

This comment has been minimized.

Copy link
@ignasi35

ignasi35 Feb 25, 2020

Member

Makes sense, but that implementation is only valid for DNS A lookups (it's a DnsNameResolver) and in that case the name and the authority will likely match. AkkaDiscoveryNameResolver may use any discovery the user wants to inject.

And it doesn't matter anyway because we're using:

    builder = settings.overrideAuthority.map(builder.overrideAuthority(_)).getOrElse(builder)

in NettyClientUtils. :-)

client.sayHello(HelloRequest(s"Hello $i")).futureValue
}

service1.greetings.get + service2.greetings.get should be(2 * requestsPerServer)

This comment has been minimized.

Copy link
@ignasi35
@raboof raboof requested a review from ignasi35 Feb 24, 2020
Akka streaming automation moved this from Ready for review to Reviewer approved Feb 25, 2020
@raboof raboof merged commit 96e1551 into master Feb 25, 2020
4 checks passed
4 checks passed
Jenkins Pull Request Validation Test PASSed. 210 tests run, 30 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details
Akka streaming automation moved this from Reviewer approved to Done Feb 25, 2020
@raboof raboof deleted the plugDiscoveryIntoNameResolver branch Feb 25, 2020
@raboof raboof mentioned this pull request Feb 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.