-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
✨ etcd: Etcd client with dialer passthrough. #2031
Conversation
d9ce606
to
d1fe78d
Compare
d1fe78d
to
1f3f3ad
Compare
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.
some architecture concerns and some copy pasta clean ups
70a1384
to
f4cd6c0
Compare
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.
Everything in this review is a nit. Feel free to fix or not. Looks great!
/approve
Thanks a lot for creating this client! /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chuckha, dlipovetsky, randomvariable The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Out of curiosity: Why create an ephemeral etcd client in each function, as opposed to creating a long-lived etcd client? |
80f64c8
to
74957ed
Compare
May need to refresh the connection as API servers move around, but it makes more sense for that to be a consumer-level concern, so have switched to long-lived. |
/retest |
74957ed
to
7c32267
Compare
Reviewing this now |
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.
Overall this is a great effort. I've left some comments regarding some consistency around the godoc lines (mostly nits), and some others regarding the logic during backoff.
Thank you for putting this together!
f9480df
to
bcb1e6b
Compare
0490393
to
4ad1568
Compare
/test pull-cluster-api-verify |
4ad1568
to
e5b2b78
Compare
70f95ac
to
bf5f646
Compare
Allows user to contact etcd with arbitrary connection types, useful when proxying through the Kubernetes API server. Protobuf types are wrapped such that downstream consumers do not need to import etcd packages. Signed-off-by: Naadir Jeewa <jeewan@vmware.com>
bf5f646
to
9add495
Compare
Rebased on master. |
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.
/lgtm
/hold
@ncdc feel free to remove the hold when you're ready
if c.MemberID == newMember.ID { | ||
newMember.Alarms = append(newMember.Alarms, c.Type) | ||
} | ||
members[i] = newMember |
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.
Don't you want this outside of the alarms loop?
} | ||
|
||
// UpdateMemberPeerList updates the given member's list of peers. | ||
func (c *Client) UpdateMemberPeerList(id uint64, peerURLs []string) (*[]Member, error) { |
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.
You want to return a pointer to a slice, not a slice of pointers?
} | ||
|
||
// Members retrieves a list of etcd members. | ||
func (c *Client) Members() (*[]Member, error) { |
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.
You want to return a pointer to a slice, not a slice of pointers?
} | ||
|
||
// Alarms retrieves all alarms on a cluster. | ||
func (c *Client) Alarms() (*[]MemberAlarm, error) { |
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.
You want to return a pointer to a slice, not a slice of pointers?
// BackoffParams sets the parameters for the client | ||
func BackoffParams(params wait.Backoff) func(*Client) error { | ||
return func(c *Client) error { | ||
return c.setBackoffParams(params) |
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'm not sure I would have split this out into a separate function, but it's fine to keep it this way if you want.
// Endpoint changes the etcd endpoint for the client | ||
func Endpoint(addr string) func(*Client) error { | ||
return func(c *Client) error { | ||
return c.setEndpoint(addr) |
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'm not sure I would have split this out into a separate function, but it's fine to keep it this way if you want.
// Timeout changes the method timeout for the etcd client | ||
func Timeout(duration time.Duration) func(*Client) error { | ||
return func(c *Client) error { | ||
return c.setTimeout(duration) |
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'm not sure I would have split this out into a separate function, but it's fine to keep it this way if you want.
Doing revisions whilst working on consumption of the package. Temporarily closing. |
What this PR does / why we need it:
Allows user to contact etcd with arbitrary connection types, useful
when proxying through the Kubernetes API server.
Protobuf types are wrapped such that downstream consumers do not
need to import etcd packages.
Signed-off-by: Naadir Jeewa jeewan@vmware.com
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Part of #1902