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

feat: retry connect to meta until meta online #3165

Merged
merged 2 commits into from
Jun 13, 2022
Merged

Conversation

yezizp2012
Copy link
Contributor

@yezizp2012 yezizp2012 commented Jun 13, 2022

What's changed and what's your intention?

As title. This PR fixes #3065, but we still need to implement gRPC health check protocol mentioned in #41.

Please explain IN DETAIL what the changes are in this PR and why they are needed:

  • Summarize your change (mandatory)
  • How does this PR work? Need a brief introduction for the changed logic (optional)
  • Describe clearly one logical change and avoid lazy messages (optional)
  • Describe any limitations of the current code (optional)
  • Add the 'user-facing changes' label if your PR contains changes that are visible to users (optional)

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

Resolve #3065 #295

@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #3165 (bc19551) into main (fb9174f) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #3165      +/-   ##
==========================================
- Coverage   73.52%   73.51%   -0.01%     
==========================================
  Files         739      739              
  Lines      101695   101710      +15     
==========================================
+ Hits        74770    74771       +1     
- Misses      26925    26939      +14     
Flag Coverage Δ
rust 73.51% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/rpc_client/src/meta_client.rs 0.00% <0.00%> (ø)
src/meta/src/barrier/mod.rs 69.45% <0.00%> (+0.32%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Contributor

@BowenXiao1999 BowenXiao1999 left a comment

Choose a reason for hiding this comment

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

I remembered that we have retry logic in Frontend/CN when connect to meta?

@yezizp2012
Copy link
Contributor Author

yezizp2012 commented Jun 13, 2022

I remembered that we have retry logic in Frontend/CN when connect to meta?

Nope, only the legacy Frontend does 😂. Currently, FE/CN will panic when meta is not online.

Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

LGTM.
BTW, I remember the purpose of moving these RPC interfaces into a separate crate is to implement a generic retry strategy for them. Any further idea on this? 🤣

@yezizp2012
Copy link
Contributor Author

LGTM. BTW, I remember the purpose of moving these RPC interfaces into a separate crate is to implement a generic retry strategy for them. Any further idea on this? 🤣

Not really. 🤣 Well I'm not sure about this. NTFS.

@yezizp2012 yezizp2012 merged commit 2aab325 into main Jun 13, 2022
@yezizp2012 yezizp2012 deleted the feat/retry-conn-meta branch June 13, 2022 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

components panic while start cluster using docker compose
3 participants