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

[doc] client channel specification #38953

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

markdroth
Copy link
Member

No description provided.

@markdroth markdroth added the release notes: no Indicates if PR should not be in release notes label Mar 8, 2025
@markdroth markdroth marked this pull request as ready for review March 8, 2025 01:05
@markdroth markdroth requested review from ejona86 and dfawley March 8, 2025 01:05
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Looks really good overall! I have lots of comments, but it's a huge document, too.

Copy link
Member Author

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough review, Doug!


#### dns Resolver

The DNS resolver accepts URIs of the form `dns:\[//authority/\]host\[:port\]`.
Copy link
Member Author

Choose a reason for hiding this comment

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

I am firmly of the opinion that Eric is wrong here. :) As far as I'm concerned, this is totally legal, reasonable, and unambiguous syntax, and it's a bug that Java doesn't support it.

nothing (if there is no service config to return).
* A resolution note string, which is used to provide context to be included
in RPC failure status messages generated by LB policies.
* A health result callback, to be invoked by the channel to indicate to the
Copy link
Member

Choose a reason for hiding this comment

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

Why is this called a "health result" callback? Can we not use "health" here, because that's confusing with our other use of "health" (as in, the service)?

Also, it seems from our meeting notes (dated 2022-02-18) that we agreed to implement this synchronously? I checked with @ejona86 and he says Java is doing it this way, because he thought that's what we agreed to. But it also says "still going to do async name resolver callback". Why? Do you do both now in C++? Is this not the same information? And 2024-04-26, "C++ does what Go does and plumbs LB result back to resolver". Is this meaning "via the callback", or synchronously?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed the wording here to avoid the word "health".

My reading of the notes from 2022-02-18 is that it's talking about two different things: we decided that getting the status back from the LB policy's Update() method to the channel would be synchronous, but we said that sending that result from the channel back to the resolver would still be async (except in Go, where the latter would be synchronous). That's where I thought we had left things, and that's what I've got implemented in C-core.

I think the outcome of the discussion on 2024-04-26 was that we didn't change anything. I think the "C++ does what Go does and plumbs LB result back to resolver" comment was just about the fact that those two languages do plumb the result back to the resolver, whereas Java wasn't yet doing that.

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

Successfully merging this pull request may close these issues.

3 participants