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.

Sign up for GitHub

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

Jump to bottom

feat: apply grpc service config from consul (#1045) #1046

Merged
merged 1 commit into from
Apr 12, 2024
Merged

feat: apply grpc service config from consul (#1045) #1046

merged 1 commit into from
Apr 12, 2024

Conversation

onyn
Copy link
Contributor

@onyn onyn commented Feb 2, 2024

Closes #1045

ST-DDT
ST-DDT requested changes Feb 3, 2024
View reviewed changes
Copy link
Collaborator

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Could you please replace var with actual types?

@ST-DDT
Copy link
Collaborator

ST-DDT commented Mar 13, 2024

Please resolve the merge conflicts.

@onyn
Copy link
Contributor Author

onyn commented Mar 13, 2024

Done. Two questions remains:

  1. Should we support canarying changes feature of DNS resolver? This may be supported in the future, using separate metadata key in case of demand.
  2. Currently GRPC doesn't expose methods for parsing json string into ConfigOrError object. Shoud we wait for it?

@onyn onyn mentioned this pull request Mar 13, 2024
Closed
@ST-DDT
Copy link
Collaborator

ST-DDT commented Mar 13, 2024

Thanks for your fast response.

Lets wait a bit on how upstream decides on the API topic.

@onyn
Copy link
Contributor Author

onyn commented Mar 25, 2024

That was expected.

So, just one question left:

  1. Should we support canarying changes feature of DNS resolver? This may be supported in the future, using separate metadata key in case of demand.
  2. Currently GRPC doesn't expose methods for parsing json string into ConfigOrError object. Shoud we wait for it?

@ST-DDT
Copy link
Collaborator

ST-DDT commented Mar 25, 2024

  1. Should we support canarying changes feature of DNS resolver?

No, just keep it simple. If grpc adds an official API, we can adopt it then.

@onyn
Copy link
Contributor Author

onyn commented Mar 26, 2024

Rebased and squashed.

@ST-DDT ST-DDT added the enhancement A feature request or improvement label Mar 26, 2024
ST-DDT
ST-DDT previously approved these changes Mar 29, 2024
View reviewed changes
Copy link
Collaborator

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Looks good to me.

...tarter/src/main/java/net/devh/boot/grpc/client/nameresolver/DiscoveryClientNameResolver.java Outdated Show resolved Hide resolved
@onyn
Copy link
Contributor Author

onyn commented Apr 2, 2024

Rebased & squashed.

@yidongnan yidongnan added this to the 3.1.0 milestone Apr 12, 2024
@yidongnan yidongnan merged commit cb029de into grpc-ecosystem:master Apr 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DNVindhya DNVindhya DNVindhya left review comments

@ST-DDT ST-DDT ST-DDT approved these changes

@yidongnan yidongnan yidongnan approved these changes

Assignees
No one assigned
Labels
enhancement A feature request or improvement
Projects
None yet
Milestone
3.1.0
Development

Successfully merging this pull request may close these issues.

Apply grpc service config from consul
4 participants
@onyn @ST-DDT @yidongnan @DNVindhya