Reduce cost of admin user check #8155
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joelegasse
previously approved these changes
Mar 17, 2017
dgnorton
previously approved these changes
Mar 17, 2017
mark-rushakoff
previously approved these changes
Mar 17, 2017
|
@joelegasse @dgnorton @mark-rushakoff please review 9d9b3254 |
joelegasse
reviewed
Mar 17, 2017
services/meta/data.go
Outdated
| @@ -576,9 +585,12 @@ func (data *Data) DropUser(name string) error { | |||
| for i := range data.Users { | |||
| if data.Users[i].Name == name { | |||
| data.Users = append(data.Users[:i], data.Users[i+1:]...) | |||
| // Maybe we dropped the only admin user? | |||
| data.adminUserExists = data.hasAdminUser() | |||
There was a problem hiding this comment.
You could add a check here to see if the user being removed is an admin, first.
There was a problem hiding this comment.
@joelegasse but there could still be another admin lurking in the background.
There was a problem hiding this comment.
I meant if the user isn't an admin, there's not a point in refreshing the cached value
dgnorton
reviewed
Mar 17, 2017
services/meta/data.go
Outdated
| @@ -576,9 +585,12 @@ func (data *Data) DropUser(name string) error { | |||
| for i := range data.Users { | |||
| if data.Users[i].Name == name { | |||
| data.Users = append(data.Users[:i], data.Users[i+1:]...) | |||
| // Maybe we dropped the only admin user? | |||
There was a problem hiding this comment.
Isn't that out of the scope of this PR though?
mark-rushakoff
approved these changes
Mar 17, 2017
services/meta/data.go
Outdated
| @@ -731,6 +751,17 @@ func (data *Data) UnmarshalBinary(buf []byte) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // hasUserExists exhaustively checks for the presence of at least one admin | |||
There was a problem hiding this comment.
s/hasUserExists/hasAdminUser
e-dard
force-pushed
the
er-user-perf
branch
2 times, most recently
from
ba14401 to
aeced4f
Compare
joelegasse
approved these changes
Mar 17, 2017
e-dard
force-pushed
the
er-user-perf
branch
2 times, most recently
from
3a57b7d to
c7214b8
Compare
This commits adds a caching mechanism to the Data object, such that when large numbers of users exist in the system, the cost of determining if there is at least one admin user will be low. To ensure that previously marshalled Data objects contain the correct cached admin user value, we exhaustively determine if there is an admin user present whenever we unmarshal a Data object.
dgnorton
approved these changes
Mar 20, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Required for all non-trivial PRs
This PR simplifies the process of checking for the existence of an admin user when handling a query.