Add the name resolver API #2285

arjan-bal · 2025-05-22T14:06:45Z

No description provided.

arjan-bal · 2025-05-22T14:20:44Z

grpc/src/client/name_resolution/backoff.rs

dfawley · 2025-05-27T20:50:26Z

grpc/src/client/name_resolution/backoff.rs

+
+    /// The delay for the next retry, without the random jitter. Store as f64
+    /// to avoid rounding errors.
+    next_delay_secs: Mutex<f64>,


Why does this need a mutex? It seems like we should generally only be accessing these serially.

You're correct access is serial. The mutex is added to allow interior mutability. @easwars and I have a discussion about this on the group. The subchannel will also use backoffs and it will pass the backoff as an immutable value to keep the subchannel API clean. To get rid of the mutex we would need to use mutable acceptors in the trait definition and have the subchannel wrap the backoff in a mutex to get a mutable ref.

It would be better to extend the mut receiver out as far as we can. The mutex can go into the subchannel if that's what it needs to do. The backoff can express that it should not be called concurrently by making this a mutable receiver instead of an immutable one. The behavior of concurrent accesses would be undefined anyway.

Removed the mutex from the backoff and used mutable receivers.

grpc/src/client/name_resolution/dns/mod.rs

grpc/src/client/name_resolution/mod.rs

dfawley · 2025-05-27T21:55:28Z

grpc/src/client/name_resolution/mod.rs

+    /// returns an empty endpoint list but a valid service config may set
+    /// to this to something like "no DNS entries found for <name>".
+    pub resolution_note: Option<String>,


Is there some requirement for using this? Like should it only be set if endpoints is empty? I.e. should we delete this and use an Err() from endpoints instead?

From my understanding, the resolution_note can be set when endpoints are present. The resolution note may say something like "the endpoints are stale" and the LB policy will add this message to the failure message if it fails to connect to the endpoints. Here is the code in CPP that sets a resolution node with a non-empty list of addresses.

Okay that makes sense.

So then the valid states we want to represent are:

endpoints service_config resolution_note

Err Err None

Err Ok None

Ok Err None

Ok Ok None

Ok Ok Some

?

I read the doc string in CPP and it states the condition for the note to be present:

/// for inclusion in RPC failure status
/// messages in cases where neither \a addresses nor \a service_config
/// has a non-OK status.

Removing the negative logic, it mentions "for inclusion in RPC failure status when both addresses and service config are Ok status". The above table is correct.

I went through the docstring in C++ and it mentions the conditions under which the resolution note is set:

in cases where neither \a addresses nor \a service_config has a non-OK status.

Removing the negative logic, we get "in cases where both addresses and service_config have an OK status." So the table above is correct.

Do you suggest creating an enum that only represents valid states?

I think we can stick with this for now since it matches C++, but we might want to change things before making a public resolver API. I always prefer when illegal states are not possible to represent in the first place, but maybe that isn't feasible here.

dfawley · 2025-05-27T21:56:37Z

grpc/src/client/name_resolution/mod.rs

+    pub resolution_note: Option<String>,
+}
+
+impl Default for ResolverUpdate {


Can this be derived? Or no because of Result? If the latter, is there some way to partially derive?

It can't be derived directly due to the Result. We can't implement the Default trait for Result<T, E> because Result is from a foreign crate. We can wrap Result<Option<ServiceConfig>, String> in our own type, but that would add an extra layer for accessing the ServiceConfig/Entpoints etc.

I couldn't find a way to avoid listing the fields that already implement Default without adding a crate with macros. The closest think I found is using Default::default() as the value for such fields (e.g: attributes: Arc::default()). Using ..Default::default() at the end of the struct in the implementation of default() causes infinite recursion.

Yeah, I think this plus my other comment about using result at a higher level kinda tie together that maybe this api is going against the grain?

grpc/src/rt/mod.rs

dfawley

LGTM but it would be great to get a review from @LucioFranco too

easwars · 2025-06-04T04:13:33Z

grpc/src/client/name_resolution/backoff.rs

+
+impl BackoffConfig {
+    fn validate(&self) -> Result<(), &'static str> {
+        // Valid that params are in valid ranges.


Nit: s/Valid/Validate?

Rephrased the sentence to avoid repeating the term "Valid".

easwars · 2025-06-04T04:18:33Z

grpc/src/client/name_resolution/mod.rs

I had a discussion about this with @dfawley a while back when we were talking about something else. https://doc.rust-lang.org/book/ch07-05-separating-modules-into-different-files.html#alternate-file-paths

Should we use the new style file paths instead? It would be easier to do that earlier than later (when we have more code). Doesn't have to happen as part of this PR, but if we have consensus, then we can fix in a follow-up PR.

@LucioFranco

We discussed this in a meeting, and Lucio said we should keep using <module_name>/mod.rs instead of <module_name.rs> plus <module_name>/<sub_modules>. (@LucioFranco, please correct me if I am misrepresenting or misremembering.)

Yeah, there is no real preference in the community over each path and historically most projects would use mod.rs and so they have stuck with it. Tonic is in the same boat, honestly, I prefer mod.rs it feels more natural rust wise. Most important thing is that we stay consistent.

easwars · 2025-06-04T04:31:04Z

grpc/src/client/name_resolution/mod.rs

+
+    /// Returns either host:port or host depending on the existence of the port
+    /// in the authority.
+    pub fn authority_host_port(&self) -> String {


Any specific reason why this method alone is returning a String while others are returning a &str?

I think it's because of the line that constructs a string below:

format!("{}:{}", host, port)

Since the string is created in the function, we can't return a ref to it because the ref will outlive the lifetime of the string.

We could consider doing something like what the http crate does https://docs.rs/http/latest/src/http/uri/authority.rs.html#261 where its internal rep is a string and then to return just the port it does some string parsing. Though this might be tough with the Url crate...

I don't see this being too much of a problem since this is called once while the channel's resolver is created. If needed, we could store the host_port as a struct field during construction to avoid an allocation on every call.

grpc/src/client/name_resolution/mod.rs

grpc/src/client/name_resolution/registry.rs

easwars · 2025-06-04T04:58:39Z

grpc/src/client/name_resolution/dns/mod.rs

+        target: &super::Target,
+        options: super::ResolverOptions,


Is there any recommendation about when to use super::Struct as opposed to having a use statement for it?

I've been using qualified names when the symbol is used once or twice and falling back to a use statement when the symbol is used more frequently. @LucioFranco wanted to get your thoughts.

I think I always use the import style and ONLY use the qualified if for example there are two structs with the same name and/or it just makes sense with the combination of the structs module name + its name in the code. In this case, super means nothing I would just import it.

Changed to use the import style throughout.

grpc/src/client/name_resolution/dns/mod.rs

easwars · 2025-06-05T16:45:46Z

Changes LGTM. I don't have required privileges to mark a comment as resolved, looks like. So, I just added a "thumbs-up" to the ones that I thought were sufficiently addressed, and left the other ones as-is.

LucioFranco · 2025-06-06T19:01:48Z

grpc/src/rt/mod.rs

+}
+
+#[derive(Default)]
+pub struct ResolverOptions {


Is the plan to expose structs with public fields?

Yes, we need to support non-tokio runtimes for a Google internal user, but this will happen after the initial preview release.

Right the question was more so about how we make fields public. By default in rust we should not make fields pub as it makes maintaining a non breaking api easier.

Should I make these fields pub(crate) for now and defer the API design decision until we decide to expose them?

That is def better, but maybe consider pub(super) to constrain any usage of the fields to things inside that module and anything you need outside you can get via function.

Changed to pub(super). I also went through all other pub struct and pub traits to reduce the scope if the API isn't going to be public.

grpc/src/client/name_resolution/registry.rs

LucioFranco · 2025-06-06T20:09:25Z

grpc/src/client/name_resolution/registry.rs

+    pub fn add_builder(&self, builder: Box<dyn ResolverBuilder>) {
+        let scheme = builder.scheme();
+        if scheme.chars().any(|c| c.is_ascii_uppercase()) {
+            panic!("Scheme must not contain uppercase characters: {}", scheme);


This should probably be an error eventually?

Resolver builders should be added when the application starts up, before any RPCs are made. So we decided that panicking here is acceptable. In the get function below, we lowercase the scheme because get is be called when RPCs are made, so we want to avoid failures.

We can also add a variant that is try_ that returns a result and then this add_ variant will just panic on that and that allows users to choose the behavior.

Added a fallible variation of the method.

grpc/src/client/name_resolution/registry.rs

LucioFranco · 2025-06-06T20:12:45Z

grpc/src/client/name_resolution/mod.rs

Yeah, there is no real preference in the community over each path and historically most projects would use mod.rs and so they have stuck with it. Tonic is in the same boat, honestly, I prefer mod.rs it feels more natural rust wise. Most important thing is that we stay consistent.

grpc/Cargo.toml

grpc/src/client/name_resolution/dns/mod.rs

arjan-bal

Sorry for the late reply, I missed the review notification because it went to my non-work email.

grpc/Cargo.toml

grpc/src/client/name_resolution/dns/mod.rs

grpc/src/client/name_resolution/registry.rs

arjan-bal · 2025-06-13T09:24:56Z

grpc/src/client/name_resolution/registry.rs

+    pub fn add_builder(&self, builder: Box<dyn ResolverBuilder>) {
+        let scheme = builder.scheme();
+        if scheme.chars().any(|c| c.is_ascii_uppercase()) {
+            panic!("Scheme must not contain uppercase characters: {}", scheme);


Resolver builders should be added when the application starts up, before any RPCs are made. So we decided that panicking here is acceptable. In the get function below, we lowercase the scheme because get is be called when RPCs are made, so we want to avoid failures.

arjan-bal · 2025-06-13T09:26:10Z

grpc/src/rt/mod.rs

+}
+
+#[derive(Default)]
+pub struct ResolverOptions {


Yes, we need to support non-tokio runtimes for a Google internal user, but this will happen after the initial preview release.

arjan-bal · 2025-06-13T09:29:14Z

grpc/src/client/name_resolution/mod.rs

+    use super::Target;
+
+    #[test]
+    pub fn parse_target() {


I copied most of these tests from gRPC Go. I haven't looked into fuzz testing and I believe only gRPC c-core does fuzz testing. I can take a look in my free time.

arjan-bal · 2025-06-13T10:04:55Z

grpc/src/client/name_resolution/mod.rs

+
+    /// The address itself is passed to the transport in order to create a
+    /// connection to it.
+    pub address: String,


Introduced a wrapper type similar to http's ByteStr and used it for the address field.

arjan-bal force-pushed the nameresolver-api branch from 2867b3f to 3c30ab8 Compare May 22, 2025 14:08

Add name resolution API

07eb017

arjan-bal force-pushed the nameresolver-api branch from 3c30ab8 to 07eb017 Compare May 22, 2025 14:14

Remove unstable deps

1f8e819

dfawley reviewed May 27, 2025

View reviewed changes

arjan-bal added 3 commits May 29, 2025 22:05

let backoff creation fail

d7724ff

Address comments in dns implementation

eeda1e6

Name resolution API changes

ec193f0

arjan-bal requested a review from dfawley May 29, 2025 18:25

remove mutex from backoff

a73c5c0

dfawley approved these changes Jun 3, 2025

View reviewed changes

easwars reviewed Jun 4, 2025

View reviewed changes

Address review comments

0a49114

arjan-bal requested a review from easwars June 4, 2025 13:57

easwars approved these changes Jun 5, 2025

View reviewed changes

LucioFranco reviewed Jun 7, 2025

View reviewed changes

Address review

9e06c21

arjan-bal commented Jun 13, 2025

View reviewed changes

arjan-bal requested a review from LucioFranco June 13, 2025 10:08

Add fallable function to register resolver

b3f1a65

LucioFranco approved these changes Jun 20, 2025

View reviewed changes

Reduce visibility, avoid qualified usages, make resolver sync

c892451

arjan-bal requested a review from LucioFranco June 26, 2025 20:12

dfawley merged commit c6760cf into hyperium:next Jun 27, 2025
16 of 17 checks passed

`endpoints`	`service_config`	`resolution_note`
`Err`	`Err`	`None`
`Err`	`Ok`	`None`
`Ok`	`Err`	`None`
`Ok`	`Ok`	`None`
`Ok`	`Ok`	`Some`

Add the name resolver API #2285

Add the name resolver API #2285

Uh oh!

Conversation

arjan-bal commented May 22, 2025

Uh oh!

arjan-bal commented May 22, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucioFranco Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LucioFranco Jun 20, 2025 •

edited

Loading