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

Add Configurable Max Retries for get_config in ConfigService #242

Merged
merged 6 commits into from
Aug 12, 2024

Conversation

451846939
Copy link
Contributor

This pull request seeks to address the issue described in #241, where the get_config method in ConfigService blocks the main thread if the connection to the configuration server fails. The main issue arises from NacosGrpcConnection’s poll_ready method, which continuously retries without a limit.

Changes Introduced:

1.	Configurable Maximum Retries: I have introduced a new optional configuration parameter, max_retries, for get_config. This parameter allows users to set a maximum number of retries for establishing a connection. The default behavior (infinite retries) remains unchanged if max_retries is not set.
2.	Modification to poll_ready: The behavior of poll_ready in NacosGrpcConnection has been modified to respect the max_retries configuration. Once the retry limit is reached, poll_ready will stop retrying and return a failure, allowing the application to proceed without blocking the main thread indefinitely.

Rationale:
Implementing a maximum retries limit offers greater control over service initialization, ensuring that services are not indefinitely blocked by failed attempts to fetch remote configurations.

@CLAassistant
Copy link

CLAassistant commented Aug 12, 2024

CLA assistant check
All committers have signed the CLA.

src/api/props.rs Outdated
@@ -26,6 +26,8 @@ pub struct ClientProps {
client_version: String,
/// auth context
auth_context: HashMap<String, String>,
/// max retries
max_retries: Option<i32>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

次数肯定非负,考虑 u32 ?与 retry_count 一同修改

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌 retry_count 我一起改为 u32

@@ -480,6 +483,13 @@ where
debug_span!(parent: None, "grpc_connection", id = self.id.clone()).entered();

loop {
if let Some(max_retries) = self.max_retries {
if self.retry_count >= max_retries {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里是提前判断了,很可能需要的是 self.retry_count > max_retries ,比如 max_retries=1 时,应该有一次重试的机会。max_retries=0 才不回有任何重试

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

Copy link
Collaborator

@CherishCai CherishCai left a comment

Choose a reason for hiding this comment

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

LGTM

@CherishCai CherishCai added the enhancement New feature or request label Aug 12, 2024
@CherishCai CherishCai merged commit 2f6265c into nacos-group:main Aug 12, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants