Skip to content
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

fix: lower threshold for system address book sync #41649

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Nov 21, 2023

Summary

  • Switch back to countUsers to have the actual value. countSeenUsers is bad if Nextcloud is connected to a larger directory, but only a part is using Nextcloud and therefore the seen count is much lower because the sync is done for all users.
  • Lower the threshold to 100

TODO

  • CI

Checklist

@kesselb kesselb added bug 3. to review Waiting for reviews labels Nov 21, 2023
@kesselb kesselb self-assigned this Nov 21, 2023
@kesselb kesselb force-pushed the bug/noid/lower-sync-threshold branch from c2ba7ab to 4bf450f Compare November 21, 2023 18:54
@solracsf solracsf added this to the Nextcloud 28 milestone Nov 21, 2023
@blizzz blizzz mentioned this pull request Nov 22, 2023
5 tasks
@@ -49,7 +49,7 @@ public function __construct(private SyncService $syncService,
* @param array $options
*/
public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void {
if($this->userManager->countSeenUsers() > 1000) {
if(array_sum($this->userManager->countUsers()) > 100) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if countSeenUsers() is faster you can do a combined condition. something like if (countSeenUsers() > 100 || countUsers() > 100)

this means countUsers is called only for user backends that don't support counting, or where seen users is lower 100 while actual users is above

@miaulalala anything I'm missing? this is what we defined a few weeks ago

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's cool 👍

countSeenUsers
select count(userid) from oc_preferences where appid = 'login' and configkey = 'lastLogin';

countUsers

Database:

public function countUsers() {
$this->fixDI();
$query = $this->dbConn->getQueryBuilder();
$query->select($query->func()->count('uid'))
->from($this->table);
$result = $query->executeQuery();
return $result->fetchOne();

SAML: https://github.com/nextcloud/user_saml/blob/b8d0ed551752d2e1ef23a8fac26424b1d067b91c/lib/UserBackend.php#L674-L677
LDAP: Is usually an external request for the ldap directory.

- Switch back to countUsers to have the actual value. countSeenUsers is bad if Nextcloud is connected to a larger directory, but only a part is using Nextcloud and therefore the seen count is much lower because the sync is done for all users.
- Lower the threshold to 100 for smaller installations.

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@kesselb kesselb force-pushed the bug/noid/lower-sync-threshold branch from 4bf450f to 7b74b07 Compare November 22, 2023 13:59
@kesselb
Copy link
Contributor Author

kesselb commented Nov 22, 2023

/backport to stable27

@AndyScherzinger AndyScherzinger merged commit e490ac6 into master Nov 22, 2023
50 checks passed
@AndyScherzinger AndyScherzinger deleted the bug/noid/lower-sync-threshold branch November 22, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants