Skip to content

新增对ServiceConfig配置的支持#574

Open
wushengju wants to merge 7 commits into
grpc-ecosystem:masterfrom
wushengju:master
Open

新增对ServiceConfig配置的支持#574
wushengju wants to merge 7 commits into
grpc-ecosystem:masterfrom
wushengju:master

Conversation

@wushengju

@wushengju wushengju commented Aug 11, 2021

Copy link
Copy Markdown

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:

@ST-DDT ST-DDT left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

ST-DDT commented Aug 23, 2021

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

@ST-DDT ST-DDT left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@ST-DDT

ST-DDT commented Jul 31, 2024

Copy link
Copy Markdown
Collaborator

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