-
Notifications
You must be signed in to change notification settings - Fork 333
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: add etcd_connect_options
for metasrv
#3853
Conversation
6b7d230
to
a272f06
Compare
|
||
/// The etcd connect options. | ||
/// Most of the options are from `etcd_client::ConnectOptions`. | ||
pub etcd_connect_options: EtcdConnectOptions, |
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.
This option itself can be Option
? Or we define a etcd
subsection for metasrv. Extending this option looks like we leak/couple the metasrv with etcd implementation.
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 seems reasonable. It also reminds me that there is more than one option for metasrv backend storage in the future(file system, memory, etcd, ...). I think we need the BackendStorageProvider
and etcd is one of the providers.
For example:
[storage]
type = "etcd"
addr = "127.0.0.1:2379"
timeout = "5s"
What do you think? @fengjiachun
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 seems reasonable. It also reminds me that there is more than one option for metasrv backend storage in the future(file system, memory, etcd, ...). I think we need the
BackendStorageProvider
and etcd is one of the providers.For example:
[storage] type = "etcd" addr = "127.0.0.1:2379" timeout = "5s"What do you think? @fengjiachun
+1
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3853 +/- ##
==========================================
- Coverage 85.60% 85.28% -0.33%
==========================================
Files 955 955
Lines 163262 163278 +16
==========================================
- Hits 139764 139245 -519
- Misses 23498 24033 +535 |
a272f06
to
cdc6a25
Compare
cdc6a25
to
69dec61
Compare
Let me refactor again for adding the |
Close it temporarily. I will re-open it when the PR is ready for review. |
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
The original
create_etcd_client()
usesNone
forConnectOptions
and can't configure some important arguments for connecting etcd.In this PR, I created the
EtcdConnectOptions
, a subset ofConnectOptions
, in theMetasrvOptions
. I also added the covert function to letEtcdConnectOptions
convertConnectOptions
easily.btw, it's convenient to add TLS configurations in the future in the same structure(I will add it in the next PR).
Checklist