-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Skip updating unchanged user settings #2128
Skip updating unchanged user settings #2128
Conversation
@andreas-p, thanks for your PR! By analyzing the history of the files in this pull request, we identified @MorrisJobke, @icewind1991 and @bartv2 to be potential reviewers. |
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.
Beside my comments this looks good 👍
@@ -212,8 +212,15 @@ public function setUserValue($userId, $appName, $key, $value, $preCondition = nu | |||
throw new \UnexpectedValueException('Only integers, floats and strings are allowed as value'); | |||
} | |||
|
|||
// TODO - FIXME | |||
$this->fixDIInit(); | |||
$curval=$this->getUserValue($userId, $appName, $key); |
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.
Could you add spaces around the equal sign.
$curval=$this->getUserValue($userId, $appName, $key); | ||
if (isset($preCondition)) { | ||
if ($curval !== $preCondition) | ||
throw new OCP\PreConditionNotMetException(); |
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.
Please add curly brackets for all if statements.
@rullzer just pointed out that it is already partially done in #1734 @andreas-p Could you rebase on top of current master and combine your changes with the current master version. |
* this PR makes sure to warm up the cache for that user * then the logic within the "if is in cache" code can be used to reduce needed queries * inspired by @andreas-p - #2128 Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Thanks a lot for your PR - while looking at it I noticed another bug in the newly introduced code. So I decided to fix this together with your improvement. See #2147 for this PR. I will close this PR here then - nevertheless: Thanks for this, because you helped to spot this issue 🙌 Feel free to maybe have a look at our starter issues: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22starter+issue%22 |
* this PR makes sure to warm up the cache for that user * then the logic within the "if is in cache" code can be used to reduce needed queries * inspired by @andreas-p - #2128 Signed-off-by: Morris Jobke <hey@morrisjobke.de>
This partially addresses #366
This small check will skip updating the user settings if the value doesn't change. Each unnecessary update will create two database roundtrips (one failed insert, one update). In case of PostgreSQL, a multitude of constraint violations is logged, and the table is bloated.
I found an average ajax call execution time reduced by 10% by suppressing unnecessary user_ldap:uid and user_ldap:displayName updates.