-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
etcdserver, auth: not cache a flag of auth status #4281
Conversation
753b136
to
0ca3ca5
Compare
This commit removes a flag that indicates auth is enabled or disabled because it doesn't have an invalidation mechanism. Fixes etcd-io#3601 and etcd-io#3964
0ca3ca5
to
b2d2c79
Compare
LGTM. I am not sure about the performance impact. (after this change, even if you do not enable auth, there is an overhead). Ideally, we should run a benchmark test. |
OK, I'll do benchmarking. Please wait for a while. |
@mitake Thanks a lot. |
I run my benchmark of 10,000 key creation (https://github.com/mitake/etcd-things/blob/master/prioritize-leader-bench/bench.go , without prioritizing leader) on the master branch and this PR. There was no significant differenct:
I think this PR doesn't introduce significant overhead because current auth flag checking doesn't require quorum, just local lookup. I combined this PR and #3994 , that requires quorum in the auth flag checking. Actually, the overhead was easily observed.
Anyway, if we integrate auth into etcdserver, consistent and low overhead auth mechanism will be possible. |
etcdserver, auth: not cache a flag of auth status
@gyuho I think this commit should be backported to the bugfix releases for reviving the auth mechanism. |
@mitake I agree. Github was down while we were back-porting commits... |
@xiang90 ah, I see :) |
This commit removes a flag that indicates auth is enabled or disabled
because it doesn't have an invalidation mechanism.
Fixes #3601 and #3964