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
refactor join cluster process #4219
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/databend/databend/EHMxJnmtTaQDE374iZYXrgapBeDa |
Thanks for the contribution! Please review the labels and make any necessary changes. |
@@ -412,6 +412,47 @@ impl MetaNode { | |||
Ok(mn) | |||
} | |||
|
|||
pub async fn join_cluster(&self, conf: &RaftConfig) -> MetaResult<()> { | |||
if conf.join.is_empty() { | |||
return Ok(()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we log a warning message when this config is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the elaborated name of this method should be try_to_join_cluster_if_cli_arg_join_is_specified()
.
conf.join
being empty means the user did not pass --join
arguments to metasrv. It's totally OK.
let addrs = &conf.join; | ||
// Join cluster use advertise host instead of listen host | ||
let raft_api_advertise_host_endpoint = conf.raft_api_advertise_host_endpoint(); | ||
#[allow(clippy::never_loop)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use addrs[0]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an unfinished code snippet... There should be a retry loop... :((
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me:DDD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested and works, thanks @lichuang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/
Summary
Changelog
Related Issues
Fixes 4218
Test Plan
Unit Tests
Stateless Tests