introduce callForSeenUsers and countSeenUsers #26361

Merged
merged 4 commits into from Oct 19, 2016

Conversation

Projects
None yet
5 participants
@butonic
Member

butonic commented Oct 12, 2016

I was running into the upgrade trying to iterate over 170000 zombie users in my two test ldap servers, when only 6 of them ever logged in. Instead of waiting 5-6 hours or maybe even longer I decided to take the time and find a better solution.

This pr introduces callForSeenUsers() and countSeenUsers() to allow only executing a callback for users that logged in at least once and as a result have a home directory that has been initialized. It also helps prevent out of memory errors.

cc @PVince81 @DeepDiver1975 there are at least two further occurences (SyncService and SyncBirthdayCalendar) of callForAllUsers in the dav app, but they seem to be related to calendar and contacts. We might add other callFor???User methods.

Backport to 9 strongly requested ❗️ !!!111einself

@butonic butonic added this to the 9.2 milestone Oct 12, 2016

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Oct 12, 2016

@butonic, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @rullzer and @DeepDiver1975 to be potential reviewers.

@butonic, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @rullzer and @DeepDiver1975 to be potential reviewers.

lib/private/Repair/RepairUnmergedShares.php
- $this->userManager->callForAllUsers($function);
+ $this->userManager->callForSeenUsers($function);

This comment has been minimized.

@PVince81

PVince81 Oct 13, 2016

Member

won't work here because even users who haven't logged in might have broken "files_target" and duplicate shares due to the fact that some sharing API calls would set up their FS...

@PVince81

PVince81 Oct 13, 2016

Member

won't work here because even users who haven't logged in might have broken "files_target" and duplicate shares due to the fact that some sharing API calls would set up their FS...

lib/private/User/Manager.php
+ * @param string $search
+ * @since 9.0.0
+ */
+ public function callForSeenUsers (\Closure $callback) {

This comment has been minimized.

@PVince81

PVince81 Oct 13, 2016

Member

please add a unit test for this. It will further confirm that it works with all DBs.

@PVince81

PVince81 Oct 13, 2016

Member

please add a unit test for this. It will further confirm that it works with all DBs.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 13, 2016

Member

Good idea !

Member

PVince81 commented Oct 13, 2016

Good idea !

+ * @return int
+ * @since 9.2.0
+ */
+ public function countSeenUsers();

This comment has been minimized.

@PVince81

PVince81 Oct 13, 2016

Member

interface change, can't backport directly

@PVince81

PVince81 Oct 13, 2016

Member

interface change, can't backport directly

This comment has been minimized.

@PVince81

PVince81 Oct 13, 2016

Member

would it be easier to simply add a flag $onlySeen = false to callForAllUsers to avoid changing the interface ?

@PVince81

PVince81 Oct 13, 2016

Member

would it be easier to simply add a flag $onlySeen = false to callForAllUsers to avoid changing the interface ?

This comment has been minimized.

@butonic

butonic Oct 13, 2016

Member

hm, that still changes the return type of countUsers() to int if $onlySeen is true. I left the new methods in the interface ... should I remove them from IUserManager completely? I dislike munching everything into a single call. We can backport it that way, but in the future I want a clean API.

The callForSeenUsers call eg, does not support searching for users because we only have the userid in the preferences. callForAllUsers iterates over the user backends and the LDAP backend may search displayname or emaul in addition to the userid.

@butonic

butonic Oct 13, 2016

Member

hm, that still changes the return type of countUsers() to int if $onlySeen is true. I left the new methods in the interface ... should I remove them from IUserManager completely? I dislike munching everything into a single call. We can backport it that way, but in the future I want a clean API.

The callForSeenUsers call eg, does not support searching for users because we only have the userid in the preferences. callForAllUsers iterates over the user backends and the LDAP backend may search displayname or emaul in addition to the userid.

This comment has been minimized.

@PVince81

PVince81 Oct 17, 2016

Member

So what to do with the interface breakage ?

In the backport you'd remove the methods from the interface ? Ok.
But then we also need to add checks everywhere if (is_callable($userManager, 'callForSeenUsers')) then call it, else fallback to the old way.

I'm fine adding these methods in 9.2.

@PVince81

PVince81 Oct 17, 2016

Member

So what to do with the interface breakage ?

In the backport you'd remove the methods from the interface ? Ok.
But then we also need to add checks everywhere if (is_callable($userManager, 'callForSeenUsers')) then call it, else fallback to the old way.

I'm fine adding these methods in 9.2.

This comment has been minimized.

@PVince81

PVince81 Oct 17, 2016

Member

Actually adding methods in 9.2 would also require a new interface IUserManager2 that extends IUserManager.

Unless we assume that no one ever bothered to implement their own user manager... there is currently no config for easy overriding without hacking server.php ...

@DeepDiver1975 thoughts ?

@PVince81

PVince81 Oct 17, 2016

Member

Actually adding methods in 9.2 would also require a new interface IUserManager2 that extends IUserManager.

Unless we assume that no one ever bothered to implement their own user manager... there is currently no config for easy overriding without hacking server.php ...

@DeepDiver1975 thoughts ?

This comment has been minimized.

@PVince81

PVince81 Oct 18, 2016

Member

Somewhere (maybe in another comment), @DeepDiver1975 answered that it is currently not possible anyway to override the interface, so changing it is ok in 9.2

@PVince81

PVince81 Oct 18, 2016

Member

Somewhere (maybe in another comment), @DeepDiver1975 answered that it is currently not possible anyway to override the interface, so changing it is ok in 9.2

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Oct 14, 2016

Member

I was thinking about this : With the user account table I want the call for uses to iterate only over known users.
Accessing not yet logged in users is a matter of search only from my pov.

Member

DeepDiver1975 commented Oct 14, 2016

I was thinking about this : With the user account table I want the call for uses to iterate only over known users.
Accessing not yet logged in users is a matter of search only from my pov.

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Oct 17, 2016

Member

@DeepDiver1975 What is the status af the user account table?

For this PR the tests are passing and it is a clear improvement. @PVince81 @DeepDiver1975 can we merge so I can backport to 9.0.?

Member

butonic commented Oct 17, 2016

@DeepDiver1975 What is the status af the user account table?

For this PR the tests are passing and it is a clear improvement. @PVince81 @DeepDiver1975 can we merge so I can backport to 9.0.?

lib/private/User/Manager.php
+ * @since 9.2.0
+ */
+ public function countSeenUsers() {
+ $limit = 10000;

This comment has been minimized.

@PVince81

PVince81 Oct 17, 2016

Member

hard-coded limit ? should we have this as argument ?

@PVince81

PVince81 Oct 17, 2016

Member

hard-coded limit ? should we have this as argument ?

This comment has been minimized.

@phisch

phisch Oct 18, 2016

Contributor

If you pass that as an argument, you will have different hard-coded values for each use. Making it harder to test. If this should be variable, we should inject it as a configuration value with propel, but i think it shouldn't.

@phisch

phisch Oct 18, 2016

Contributor

If you pass that as an argument, you will have different hard-coded values for each use. Making it harder to test. If this should be variable, we should inject it as a configuration value with propel, but i think it shouldn't.

lib/private/User/Manager.php
+ * @since 9.0.0
+ */
+ public function callForSeenUsers (\Closure $callback) {
+ $limit = 1000;

This comment has been minimized.

@PVince81

PVince81 Oct 17, 2016

Member

hardcoded limit ? pass as argument ?

Also, in the count method you hard-coded to 10000 while here only 1000. Any reason ?

@PVince81

PVince81 Oct 17, 2016

Member

hardcoded limit ? pass as argument ?

Also, in the count method you hard-coded to 10000 while here only 1000. Any reason ?

This comment has been minimized.

@butonic

butonic Oct 17, 2016

Member

gut feeling. count only iterates. call executes a callback ... who knows what is done in there and how much memory is leaked

@butonic

butonic Oct 17, 2016

Member

gut feeling. count only iterates. call executes a callback ... who knows what is done in there and how much memory is leaked

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Oct 18, 2016

Member

@PhilippSchaffrath could you review, pls?

Member

butonic commented Oct 18, 2016

@PhilippSchaffrath could you review, pls?

lib/private/User/Manager.php
*/
- public function countUsers() {
+ public function countUsers($onlySeen = false) {

This comment has been minimized.

@phisch

phisch Oct 18, 2016

Contributor

The domain is a little inconsistent here, i would like $onlySeen to be called something like $hasLoggedIn.

@phisch

phisch Oct 18, 2016

Contributor

The domain is a little inconsistent here, i would like $onlySeen to be called something like $hasLoggedIn.

lib/private/User/Manager.php
- public function countUsers() {
+ public function countUsers($onlySeen = false) {
+ if ($onlySeen) {
+ return $this->countSeenUsers();

This comment has been minimized.

@phisch

phisch Oct 18, 2016

Contributor

same here

@phisch

phisch Oct 18, 2016

Contributor

same here

This comment has been minimized.

@butonic

butonic Oct 18, 2016

Member

countHasLoggedInUsers()? looks ugly but clarifies, hm k.

@butonic

butonic Oct 18, 2016

Member

countHasLoggedInUsers()? looks ugly but clarifies, hm k.

This comment has been minimized.

@butonic

butonic Oct 18, 2016

Member

owncloud/windows_network_drive#51 has already been merged we can change the name after getting this in.

@butonic

butonic Oct 18, 2016

Member

owncloud/windows_network_drive#51 has already been merged we can change the name after getting this in.

This comment has been minimized.

@phisch

phisch Oct 18, 2016

Contributor

I don't want to block this, it's just a reminder to keep an easy and consistent domain so it's more easy to understand the code.

@phisch

phisch Oct 18, 2016

Contributor

I don't want to block this, it's just a reminder to keep an easy and consistent domain so it's more easy to understand the code.

- continue;
+ public function callForAllUsers(\Closure $callback, $search = '', $onlySeen = false) {
+ if ($onlySeen) {
+ $this->callForSeenUsers($callback);

This comment has been minimized.

@phisch

phisch Oct 18, 2016

Contributor

I would like to see a separation here. callForAllUsers should always call a callback for all users, not sometimes if parameter x is set to y only a subset of all users. Since callForSeenUsers already exists, why does it need to be cascaded in here?

@phisch

phisch Oct 18, 2016

Contributor

I would like to see a separation here. callForAllUsers should always call a callback for all users, not sometimes if parameter x is set to y only a subset of all users. Since callForSeenUsers already exists, why does it need to be cascaded in here?

This comment has been minimized.

@butonic

butonic Oct 18, 2016

Member

@PVince81 and @DeepDiver1975 wanted a param to make backporting easier since we don't backport API changes ...

@butonic

butonic Oct 18, 2016

Member

@PVince81 and @DeepDiver1975 wanted a param to make backporting easier since we don't backport API changes ...

+ $user4->delete();
+ }
+
+ public function testCallForSeenUsers() {

This comment has been minimized.

@phisch

phisch Oct 18, 2016

Contributor

smart test, like it 👍

@phisch

phisch Oct 18, 2016

Contributor

smart test, like it 👍

@DeepDiver1975

Actually adding methods in 9.2 would also require a new interface IUserManager2 that extends IUserManager.

This is only necessary for interfaces where we allow/expect alternative implementations out there.
I see no reason why an alternative implementation would make sense - furthermore there is no way to register one.

lib/private/User/Manager.php
+ /**
+ * @param \Closure $callback
+ * @param string $search
+ * @since 9.0.0

This comment has been minimized.

This comment has been minimized.

@PVince81

PVince81 Oct 18, 2016

Member

@butonic seems you missed this one

@PVince81

PVince81 Oct 18, 2016

Member

@butonic seems you missed this one

lib/private/User/Manager.php
+ * @return array with (string[])'userids' and (int)'rowcount'
+ */
+ private function getSeenUserIds($limit = 1000, $offset = 0) {
+ $queryBuilder = \OC::$server->getDatabaseConnection()->getQueryBuilder();

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Oct 18, 2016

Member

inject

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Oct 18, 2016

Member

I addressed most issues and also removed the sql hack for oracle because it was unnecessary to order the results in fhe first place.

@DeepDiver1975 @PVince81 @PhilippSchaffrath please review

Member

butonic commented Oct 18, 2016

I addressed most issues and also removed the sql hack for oracle because it was unnecessary to order the results in fhe first place.

@DeepDiver1975 @PVince81 @PhilippSchaffrath please review

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 18, 2016

Member

Looks good to me, also tested with 200 LDAP users where only 4 logged in and saw that only 4 got processed for the avatar move.

@butonic please adjust the "since" tag from above and this is good from my POV 👍

Member

PVince81 commented Oct 18, 2016

Looks good to me, also tested with 200 LDAP users where only 4 logged in and saw that only 4 got processed for the avatar move.

@butonic please adjust the "since" tag from above and this is good from my POV 👍

@butonic

This comment has been minimized.

Show comment
Hide comment
Member

butonic commented Oct 18, 2016

@PVince81 done

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
Member

DeepDiver1975 commented Oct 19, 2016

👍

@DeepDiver1975 DeepDiver1975 merged commit 0f8bf00 into master Oct 19, 2016

4 checks passed

Scrutinizer 6 new issues, 8 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@DeepDiver1975 DeepDiver1975 deleted the callforusersthatloggedin branch Oct 19, 2016

@MorrisJobke MorrisJobke referenced this pull request in nextcloud/server Oct 24, 2016

Merged

introduce callForSeenUsers and countSeenUsers #1888

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 7, 2016

Member

stable9.1: #27248
stable9: #26415

Member

PVince81 commented Dec 7, 2016

stable9.1: #27248
stable9: #26415

jvillafanez added a commit that referenced this pull request Feb 24, 2017

introduce callForSeenUsers and countSeenUsers (#26361)
* introduce callForSeenUsers and countSeenUsers

* add tests

* oracle should support not null on clob

* since 9.2.0

Conflicts:
	lib/private/Repair/MoveAvatarOutsideHome.php
	lib/private/User/Manager.php
	tests/lib/User/ManagerTest.php

@jvillafanez jvillafanez referenced this pull request Feb 24, 2017

Merged

Callforusersthatloggedin stable9.1 #27248

1 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment