-
Notifications
You must be signed in to change notification settings - Fork 54
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: endpoint choose by health check #109
Conversation
@ Yiyiyimu welcome to look at this PR |
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.
we need to update the file https://github.com/api7/lua-resty-etcd/blob/master/rockspec/lua-resty-etcd-master-0.1-0.rockspec#L20 too
Add: t/v2/dir.t is unstable. |
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, @spacewander would you have time to look at this PR?
@Yiyiyimu welcome to look at this PR too |
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.
Most code logic LGTM, but some grammar needs to be refined
|
||
|
||
|
||
=== TEST 8: has no healthy etcd endpoint, follow old style |
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 didn't get why when no endpoints are healthy, we should follow old style. Since as #101 talked about
When all instances of a certain api fail (such as auth api), it will cause crazy retries, which may eventually overwhelm the ETCD cluster.
Following old styles still could not solve this problem
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 can't refute this example.
we do not request etcd when all etcd instances are down.
what do you think @membphis ?
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.
IMO maybe we could leave some warnings and like add some delay for continuous retries? I'm not so sure.
Hi @nic-chen do you have some opinions on this
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.
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.
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.
ping @tzssangglass |
later, my computer is broken, will continue after repairing. |
…ty-etcd into healthcheck Conflicts: health_check.md
fix #101
fix #55