Skip to content

fix searching all users in repair regenerate birthday cal reparir job #15141

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.

Sign up for GitHub

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

Jump to bottom
Merged
rullzer merged 2 commits into from
Apr 24, 2019
Merged

fix searching all users in repair regenerate birthday cal reparir job #15141

rullzer merged 2 commits into from
Apr 24, 2019

Conversation

@blizzz
Copy link
Member

@blizzz blizzz commented Apr 17, 2019

The problem: my dev system on upgrade is working on that for almost an hour now…

  • this avoids searches across all users on all backends
  • most, if not all, are known anyhow
  • for those that aren't there is no reason to act upon
  • with only known users it will be tremendeously faster… but it would still be best if this could be done in a background job instead and not blocking upgrade

@blizzz blizzz added this to the Nextcloud 17 milestone Apr 17, 2019
@blizzz blizzz changed the title avoids users searches on backend, faster processing fix searching all users in repair regenerate birthday cal reparir job Apr 17, 2019
@georgehrke
Copy link
Member

georgehrke commented Apr 17, 2019

Would it be better to replace this as well?

server/apps/dav/lib/Controller/BirthdayCalendarController.php

Line 96 in 5382eec

$this->userManager->callForAllUsers(function(IUser $user) {

@blizzz
Copy link
Member Author

blizzz commented Apr 17, 2019

Would it be better to replace this as well?

server/apps/dav/lib/Controller/BirthdayCalendarController.php

Line 96 in 5382eec
$this->userManager->callForAllUsers(function(IUser $user) {

aye

@blizzz
Copy link
Member Author

blizzz commented Apr 17, 2019

and found two other spots

@blizzz
avoids users searches on backend, faster processing
1d1c8b4 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the fix/noid/regenerate-seen-users-only branch from b02b6ca to 299f4d5 Compare April 17, 2019 13:55
@blizzz
Copy link
Member Author

blizzz commented Apr 17, 2019

@georgehrke but is there a reason to have it a repair step, not a background job that can be run once after upgrade is done?

@blizzz
Copy link
Member Author

blizzz commented Apr 23, 2019

Anyway, i think we should merge and backport it, otherwise setups with a serious backend will have a long lasted upgrade. For .1 we can perhaps turn it to background job if possible.

georgehrke
georgehrke reviewed Apr 23, 2019
View reviewed changes
apps/dav/lib/CardDAV/SyncService.php
public function syncInstance(\Closure $progressCallback = null) {
$systemAddressBook = $this->getLocalSystemAddressBook();
$this->userManager->callForAllUsers(function($user) use ($systemAddressBook, $progressCallback) {
$this->userManager->callForSeenUsers(function($user) use ($systemAddressBook, $progressCallback) {
Copy link
Member

@georgehrke georgehrke Apr 23, 2019

Choose a reason for hiding this comment

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

@schiessle Will this have any negative effect on federation?

Copy link
Member Author

@blizzz blizzz Apr 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

why should this affect federation? allForSeenUsers cycles over all known users. callForAllUsers might bring in new ones, but in any case it brings in a lot of overhead.

Copy link
Member

@georgehrke georgehrke Apr 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

syncInstance syncs the address book containing all users for federation. So instead of all users only users that have logged in before will be synced (i assume).

Copy link
Member Author

@blizzz blizzz Apr 23, 2019

Choose a reason for hiding this comment

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

No, all the users known to Nextcloud. This can be more than have logged in.

@georgehrke
Copy link
Member

georgehrke commented Apr 23, 2019

but is there a reason to have it a repair step, not a background job that can be run once after upgrade is done?

The migration step doesn't generate all the birthday calendars itself, it only registers jobs to generate them later on in the background.

@blizzz blizzz requested a review from schiessle April 23, 2019 10:20
@blizzz
Copy link
Member Author

blizzz commented Apr 23, 2019

The migration step doesn't generate all the birthday calendars itself, it only registers jobs to generate them later on in the background.

Then, this should be fine. Search external backends is expensive.

@blizzz blizzz force-pushed the fix/noid/regenerate-seen-users-only branch from 299f4d5 to 44a67ce Compare April 24, 2019 13:51
@blizzz
remove obsolete use statements
96bab4f 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the fix/noid/regenerate-seen-users-only branch from 44a67ce to 96bab4f Compare April 24, 2019 14:25
@blizzz
Copy link
Member Author

blizzz commented Apr 24, 2019

/backport to stable16

@rullzer rullzer merged commit 3394665 into master Apr 24, 2019
@rullzer rullzer deleted the fix/noid/regenerate-seen-users-only branch April 24, 2019 17:58
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Apr 24, 2019
Merged
@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Apr 24, 2019

backport to stable16 in #15219

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@georgehrke georgehrke georgehrke approved these changes

@rullzer rullzer rullzer approved these changes

@MorrisJobke MorrisJobke Awaiting requested review from MorrisJobke

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

@schiessle schiessle Awaiting requested review from schiessle

Assignees

No one assigned

Labels

3. to review Waiting for reviews bug

Projects

None yet

Milestone

Nextcloud 17

Development

Successfully merging this pull request may close these issues.

4 participants

@blizzz @georgehrke @rullzer