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

Service configs in DNS. #5

Merged
merged 13 commits into from Mar 28, 2017

Conversation

Projects
None yet
7 participants
@markdroth
Copy link
Member

commented Jan 19, 2017

@markdroth markdroth requested review from carl-mastrangelo, a11r and y-zeng Jan 19, 2017

@googlebot googlebot added the cla: yes label Jan 19, 2017

A2.md Outdated
# The service config data object for clients that match the above
# criteria. (The format for this object is defined in
# https://github.com/grpc/grpc/blob/master/doc/service_config.md.)

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 20, 2017

Member

make this a proper markdown link?

This comment has been minimized.

Copy link
@markdroth

markdroth Jan 23, 2017

Author Member

It doesn't look like there's an easy way to do that inside of a triple-backtick block.

A2.md Outdated
# The service config data object for clients that match the above
# criteria. (The format for this object is defined in
# https://github.com/grpc/grpc/blob/master/doc/service_config.md.)
'serviceConfig': object,

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 20, 2017

Member

json doesn't have trailing commas

This comment has been minimized.

Copy link
@markdroth

markdroth Jan 23, 2017

Author Member

Fixed.

@therc therc referenced this pull request Jan 24, 2017

Closed

Add support for TXT entries #38

A2.md Outdated
determine which choice will be selected by a given client:

```
# A list of one or more service config choices.

This comment has been minimized.

Copy link
@ejona86

ejona86 Jan 30, 2017

Member

nit: While this isn't actually JSON, JSON (when it supports comments) uses // for comments. You could then also use the ```json for the start of the block to get syntax highlighting.

This comment has been minimized.

Copy link
@markdroth

markdroth Jan 30, 2017

Author Member

Thanks, didn't know that. Fixed.

This comment has been minimized.

Copy link
@markdroth

markdroth Jan 30, 2017

Author Member

The ```json thing didn't work right, so I removed it.

A2.md Outdated
### Encoding in DNS TXT Records

In DNS, the service config data (in the form documented in the previous
section) will be encoded in a TXT record with the attribute name

This comment has been minimized.

Copy link
@ejona86

ejona86 Jan 30, 2017

Member

You should probably call out some references of how this is done for long configs, since <character-string> is limited to 255 bytes. Specifically, that multiple strings in a single TXT RR are concatenated without space (SPF spec has most clear description). I see that mentioned in passing later, but it should be in this section, since this defines the design.

It seems like it may be prudent to reference https://tools.ietf.org/html/rfc1464 as well, either to describe that's the format we're using or explicitly saying we aren't following it.

This comment has been minimized.

Copy link
@markdroth

markdroth Jan 30, 2017

Author Member

Thanks for the references. I was aware of them but hadn't added them to the doc, although I should have. Fixed.

@ejona86

This comment has been minimized.

Copy link
Member

commented on A2.md in adafae7 Jan 30, 2017

WOW. JSON makes it look ugly (all the red). I guess you could use "javascript" instead or cut our losses and go back to no syntax highlighting.

This comment has been minimized.

Copy link
Member

replied Jan 30, 2017

It's probably all red because there are no comments in JSON.

This comment has been minimized.

Copy link
Member

replied Jan 30, 2017

I'm aware. See #5 (comment) . Comments are a pretty common extension to JSON, starting since JSON as a configuration language became a thing. Same with the trailing newline; not JSON, but a common extension.

A2.md Outdated
However, there can be multiple strings, which will be
concatenated together, as described in [RFC-4408 section
3.1.3](https://tools.ietf.org/html/rfc4408#section-3.1.3). The total
across all strings is limited to 65536 bytes. (See the "Open Issues"

This comment has been minimized.

Copy link
@ejona86

ejona86 Jan 30, 2017

Member

nit: The max size of the entire response is 65535 (not 65536). And in addition to overheads in the header there is also the byte-long length-prefix of each string (maxed out at 256 bytes).

I liked how at the end you mentioned the limit, but in a fuzzy way. Could this sentence be made more fuzzy?

This comment has been minimized.

Copy link
@markdroth

markdroth Jan 30, 2017

Author Member

Done.

@markdroth markdroth referenced this pull request Feb 13, 2017

Merged

gRFC A6: Retries #12

A2.md Outdated
config choices. For example, here is an example TXT record:

```
myserver 3600 TXT "grpc_config=[{'serviceConfig':{'loadBalancingPolicy':'round_robin','methodConfig':[{'name':[{'service':'MyService','method':'Foo'}],'waitForReady':true}]}}]"

This comment has been minimized.

Copy link
@lukaszx0

lukaszx0 Feb 23, 2017

Contributor

Why 3600 ? Is this recommended (required?) value or just an example? In either case, probably should be state explicitly.

This comment has been minimized.

Copy link
@markdroth

markdroth Feb 23, 2017

Author Member

It's just an example TTL value. There's no requirement for any particular value here (any more than there is for the content of the TXT record itself).

A2.md Outdated
//
// Client language(s): a list of strings (e.g., 'c++', 'java', 'go',
// 'python', etc).
'clientLanguage': [string],

This comment has been minimized.

Copy link
@lukaszx0

lukaszx0 Feb 23, 2017

Contributor

I wouldn't find this option very useful, in fact on a big scale with hundreds of services such knowledge is very often not needed nor it is relevant. I'd just remove this as it doesn't seem to be very helpful, but that's my personal opinion.

Having this based on some sort of capabilities exposed by servers would be nice but I'm not sure how in this particular case we could design this to be generic and usable for everybody.

This comment has been minimized.

Copy link
@markdroth

markdroth Feb 23, 2017

Author Member

The main reason for including it is that there may be cases where there is a bug in the gRPC implementation for a particular language that prevents us from enabling a certain feature for clients in that language, but we may want to enable the feature for clients in other languages while we wait for the bug to be fixed.

This is a very easy thing to support, and it doesn't impose any overhead if you don't use it, so there's no real reason not to support it.

This comment has been minimized.

Copy link
@lukaszx0

lukaszx0 Feb 23, 2017

Contributor

That's fair. Thanks!

@lukaszx0

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2017

@markdroth wrote:

[...] would prevent you from using DNS for service configs? [...]

My preferred solution would be using Zookeeper and incorporating service config as part of service discovery system (which is, I'm guessing, exactly what you guys do at Google with Chubby and we we will do internally with ZK). The main difference is that ZK is push based whereas DNS is pull which means that TTLs would have to be set low and it'd generate a huge volume of DNS traffic which might be problematic and have a lot of side effects. For most users this should be probably fine although potentially might cause some weird dns caching etc.

That said, the proposal doesn't mention how/if the implementations should refresh cached DNS TXT records. It also doesn't mention at all how TTLs should be configured, what are the recommendations etc. It's probably author's assumption that future users are familiar with nitty gritty details of DNS but since this will be the default open source implementation, I think it should be explained a bit more.

@markdroth
Copy link
Member Author

left a comment

The usage of TTLs for TXT records is no different than it is for A records, which people are already using. Whenever we look up A records, we will also look up the corresponding TXT records, so the existing behavior is not modified by this proposal.

As it happens, the existing DNS resolver implementation does not honor TTLs at all; it only re-resolves the name when the client sees an error on the channel. I do think that we should probably change that behavior, but I'd prefer to discuss that separately from this design.

Thanks for your input!

A2.md Outdated
config choices. For example, here is an example TXT record:

```
myserver 3600 TXT "grpc_config=[{'serviceConfig':{'loadBalancingPolicy':'round_robin','methodConfig':[{'name':[{'service':'MyService','method':'Foo'}],'waitForReady':true}]}}]"

This comment has been minimized.

Copy link
@markdroth

markdroth Feb 23, 2017

Author Member

It's just an example TTL value. There's no requirement for any particular value here (any more than there is for the content of the TXT record itself).

A2.md Outdated
//
// Client language(s): a list of strings (e.g., 'c++', 'java', 'go',
// 'python', etc).
'clientLanguage': [string],

This comment has been minimized.

Copy link
@markdroth

markdroth Feb 23, 2017

Author Member

The main reason for including it is that there may be cases where there is a bug in the gRPC implementation for a particular language that prevents us from enabling a certain feature for clients in that language, but we may want to enable the feature for clients in other languages while we wait for the bug to be fixed.

This is a very easy thing to support, and it doesn't impose any overhead if you don't use it, so there's no real reason not to support it.

@ejona86

This comment has been minimized.

Copy link
Member

commented Feb 23, 2017

@lukaszx0 wrote:

My preferred solution would be using Zookeeper and incorporating service config as part of service discovery system

Service discovery is name-resolver specific. If you are using Zookeeper for name resolution then it would also be able to return a service config. This document is strictly for users using the default DNS-based name resolver. So there shouldn't be any problem.

@lukaszx0

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2017

@ejona86 absolutely. I was just answering @markdroth question why I wouldn't use DNS in a first place :)

@a11r
Copy link
Contributor

left a comment

Thanks for the proposal. I have left some comments and clarification questions.

The [service
config](https://github.com/grpc/grpc/blob/master/doc/service_config.md)
mechanism was originally designed for use inside of Google. However,
all but one part of the original design will work fine in the open-source

This comment has been minimized.

Copy link
@a11r

a11r Mar 21, 2017

Contributor

s/will work fine/works fine/

This comment has been minimized.

Copy link
@markdroth

markdroth Mar 22, 2017

Author Member

Done.

mechanism was originally designed for use inside of Google. However,
all but one part of the original design will work fine in the open-source
world. That one part is the specification of how the service config
data will be encoded in DNS.

This comment has been minimized.

Copy link
@a11r

a11r Mar 21, 2017

Contributor

I would suggest being a little more explicit in the second sentence. SOmething like .. "That one part is the specification of how the service config data will be encoded in DNS. This proposal fills-in this last missing piece."

This comment has been minimized.

Copy link
@markdroth

markdroth Mar 22, 2017

Author Member

Done.

// Percentage: integer from 0 to 100 indicating the percentage of
// clients that should use this choice.
'percentage': number,
// Client hostname(s): a list of strings.

This comment has been minimized.

Copy link
@a11r

a11r Mar 21, 2017

Contributor

Is this going to be perfix-matched or exact-matched ?

Does it make sense to use a more general "Client name" instead of a "Client hostname". That would allow installations where clients get explicit names , independent of the host name, to use finer granularity.

This comment has been minimized.

Copy link
@markdroth

markdroth Mar 22, 2017

Author Member

The client hostname will be an exact match. In the discussion thread, we considered making this be a wildcard match of some sort, but it's not clear that there's actually a use-case for that right now, so we decided to just stick with exact-match semantics for now. We can always add a new type of selection criteria later, if/when we have a real use-case for it.

I'm not sure what you mean by a client getting an explicit name other than a hostname. We could certainly add something like that later, but I'd like to see a real use-case for it before doing so.

@a11r a11r merged commit c98f9cf into grpc:master Mar 28, 2017

1 check passed

cla/google All necessary CLAs are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.