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

Expose service config parsing logic #11008

Closed
onyn opened this issue Mar 13, 2024 · 2 comments
Closed

Expose service config parsing logic #11008

onyn opened this issue Mar 13, 2024 · 2 comments
Assignees

Comments

@onyn
Copy link

onyn commented Mar 13, 2024

Is your feature request related to a problem?

DNSNameResolver have service config parsing logic which is hidden from outer world.

Please expose json string to ConfigOrError parser. This simplifies implementation of A2 proposal by custom name resolvers.

Describe the solution you'd like

Implementation may be moved to NameResolver class with protected visibility level.

Also consider providing two methods:

  1. One for parsing A2's canarying changes json format (list of objects).
  2. Second for parsing just single service config itself.

Additional context

This feature request inspired by grpc-ecosystem/grpc-spring#1046

@sergiitk
Copy link
Member

@ejona86 - need your opinion on this.

@ejona86
Copy link
Member

ejona86 commented Mar 21, 2024

Second for parsing just single service config itself.

"parse" can mean a few things here. The only way to get a ConfigOrError from within a NameResolver is args.getServiceConfigParser().parseServiceConfig(Map<String,?>). That's the method you want to use.

Now to parse the JSON into Map<String,?>, we purposefully don't have anything in our API. We don't want to expose a JSON implementation. But you should be able to use the JSON implementation of your choice. For Gson, it's about as easy as Gson.fromJson(json, Map.class). That's what one example does.

So I don't see much need to do anything here.

One for parsing A2's canarying changes json format (list of objects).

This is specific to DNS. We could maybe expand it to others, but DNS still doesn't have service config enabled by default... We were expecting other name resolvers to use some of their own methods for canarying. But all clients receiving the same results does seem like it would be frequent enough that other NRs might want it to select the config to use in the same way. The implementation interesting to other NRs would mostly be maybeChooseServiceConfig().

But the current implementation also suffers from #6579 . We'd need to resolve that before having more people use it.

@sergiitk sergiitk added the Waiting on reporter there was a request for more information without a response or answer or advice has been provided label Mar 21, 2024
@onyn onyn closed this as completed Mar 25, 2024
@sergiitk sergiitk removed the Waiting on reporter there was a request for more information without a response or answer or advice has been provided label Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants