*: avoid some unnecessary call of ensureActiveUser() #57388
Conversation
ti-chi-bot
bot
added
release-note-none
|
Hi @tiancaiamao. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #57388 +/- ##
================================================
+ Coverage 73.2116% 73.9749% +0.7633%
================================================
Files 1675 1683 +8
Lines 461921 466624 +4703
================================================
+ Hits 338180 345185 +7005
+ Misses 102981 100655 -2326
- Partials 20760 20784 +24
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| drop user if exists 'testflush'@'localhost'; | ||
| CREATE USER 'testflush'@'localhost' IDENTIFIED BY ''; | ||
| SHOW GRANTS FOR 'testflush'@'localhost'; | ||
| UPDATE mysql.User SET Select_priv='Y' WHERE User="testflush" and Host="localhost"; | ||
| connect (conn1, localhost, testflush,,); | ||
| --error 1142 |
There was a problem hiding this comment.
This is a behavior change.
In the past, all the user data are in-memory, and login will not trigger reload.
Now login will trigger the reload if the user is not loaded, login make the user get the latest privileges.
To test the old behavior, SHOW GRANTS FOR 'testflush'@'localhost'; this line is added.
This cause the user data been loaded
Then login find the user and does not trigger reload.
tiancaiamao
requested review from
lcwangchao
and removed request for
Defined2014
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
|
/retest |
|
@tiancaiamao: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| @@ -1845,7 +1847,6 @@ func (e *ShowExec) fetchShowCreateUser(ctx context.Context) error { | |||
| require = privValue.RequireStr() | |||
| } | |||
|
|
|||
| authData := checker.GetEncodedPassword(ctx, e.User.Username, e.User.Hostname) | |||
There was a problem hiding this comment.
I don't quite get why we don't need ensureActiveUser() in fetchShowCreateUser and other functions. Could you please explain it a little bit, and how can we be sure the correctness can be verified?
There was a problem hiding this comment.
show create user is not show grants for user
We just need the command for creating that user, we do not need to assign any privileges to the user. @bb7133
| @@ -1795,10 +1795,6 @@ func (e *SimpleExec) executeAlterUser(ctx context.Context, s *ast.AlterUserStmt) | |||
| failedUsers = append(failedUsers, user) | |||
| continue | |||
| } | |||
| currentAuthPlugin, err := privilege.GetPrivilegeManager(e.Ctx()).GetAuthPlugin(ctx, spec.User.Username, spec.User.Hostname) | |||
There was a problem hiding this comment.
The authPlugin and authentication data is obtained by the ExecInternalSQL , read from the mysql.user.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bb7133, lcwangchao 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 |
[LGTM Timeline notifier]Timeline:
|
|
@tiancaiamao: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@tiancaiamao: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
|
@tiancaiamao: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What problem does this PR solve?
Issue Number: ref #55563
Problem Summary:
What changed and how does it work?
In #57042, I add too much calls of
ensureActiveUser()than necessary. That's introduced to make the CI pass.That PR had already done a lot to make the CI pass .... So I don't like to introduce extra complexity to it.
After merge that, this commit will be used to reduce some unnecessary
ensureActiveUser().Check List
Tests
For example
Without optimizing the unnecessary
ensureActiveUser(), If I useset password for user%dto change password for 2M users, here is the QPSAfter the optimization:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.