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

新增对ServiceConfig配置的支持 #574

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

Conversation

wushengju
Copy link

@wushengju wushengju commented Aug 11, 2021

1.新增之后需要进行如下所示的配置
grpc.client.tcl-cloud-provider.retry-enabled=true grpc.client.tcl-cloud-provider.method-config[0].name[0].service=helloworld.Greeter grpc.client.tcl-cloud-provider.method-config[0].name[0].method=SayHello grpc.client.tcl-cloud-provider.method-config[0].retry-policy.max-attempts=3 grpc.client.tcl-cloud-provider.method-config[0].retry-policy.initial-backoff=1 grpc.client.tcl-cloud-provider.method-config[0].retry-policy.max-backoff=1 grpc.client.tcl-cloud-provider.method-config[0].retry-policy.backoff-multiplier=2 grpc.client.tcl-cloud-provider.method-config[0].retry-policy.retryable-status-codes=UNKNOWN,UNAVAILABLE
2.测试结果如下
image


EDIT by @ST-DDT

English

References:

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.

Thanks for your contribution.

There are some small changes I would like to see before merging this. See my comments for details.
Also please run ./gradle spotlessApply before committing.
Lastly, please add a test that ensures this works as expected and does not break unnoticed in a future release.

If you need help with any of these, feel free to ask me and I will try to help you.

@ST-DDT ST-DDT added the enhancement A feature request or improvement label Aug 12, 2021
@wushengju
Copy link
Author

Thanks for your contribution.

There are some small changes I would like to see before merging this. See my comments for details.
Also please run ./gradle spotlessApply before committing.
Lastly, please add a test that ensures this works as expected and does not break unnoticed in a future release.

If you need help with any of these, feel free to ask me and I will try to help you.

I have settled your proposal and also add GrpcChannelPropertiesGivenMethodConfigUnitTest for test

@ST-DDT ST-DDT self-assigned this Aug 23, 2021
@ST-DDT
Copy link
Collaborator

ST-DDT commented Aug 23, 2021

I'll try to add some integration tests that actually test, that the retry/service config is working.
Maybe I can also tweak the datatypes a bit to simplify the configuration some more.

@wushengju
Copy link
Author

I'll try to add some integration tests that actually test, that the retry/service config is working.
Maybe I can also tweak the datatypes a bit to simplify the configuration some more.

Ok just do it

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.

I pushed the first part and will hopefully finish it in the next few days.

TODO for me: Check whether the config metadata will be generated by spring.

if (properties.isRetryEnabled()) {
builder.enableRetry();
// build retry policy by default service config
// TODO: Wrap field in defaultServiceConfig
Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid conflicts with other values in the serviceConfig it might be neccessary to wrap the methodConfig property in a defaultServiceConfig property for clarity.

builder.enableRetry();
// build retry policy by default service config
// TODO: Wrap field in defaultServiceConfig
builder.defaultServiceConfig(buildDefaultServiceConfig(properties));
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default service config might be used outside of the retry logic.
So it might be neccessary to move it "elsewhere".

@o-shevchenko
Copy link
Contributor

Hey
Are there any plans to merge this PR? These configurations are crucial for making our clients resilient.
Thanks!

@ST-DDT
Copy link
Collaborator

ST-DDT commented Jul 31, 2024

Currently there appears to be merge conflicts, so this cannot be merged right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature request or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

如何通过grpc-spring-boot-starter设置重试相关的配置,目前不提供这样的功能吗?
3 participants