Skip to content

Fix lookup server connector background jobs problem - #15143

Merged
rullzer merged 6 commits into
masterfrom
bugfix/noid/lookup-server-connector-background-jobs-problem
Jul 18, 2019
Merged

Fix lookup server connector background jobs problem#15143
rullzer merged 6 commits into
masterfrom
bugfix/noid/lookup-server-connector-background-jobs-problem

Conversation

@nickvergessen

@nickvergessennickvergessen commented Apr 17, 2019

Copy link
Copy Markdown
Member

Some 50 user instances have 400k pending jobs for lookup updates
The first problem is that the job was scheduled too often with #15020
The second one is that each time any value changed, a new job was scheduled (because at least the retry count and the last execution was different).
So you ended up with 6 jobs per change on user data.

As LDAP seems to set the user name/email some more times, it basically added the 6 jobs on each login?

So now the app got a little update and in the end can only have 1 background job per user.
Existing update requests are killed when the userId is not set in the arguments, so the table should clean up on the next few cron runs after the update.

@nickvergessennickvergessen added this to the Nextcloud 17 milestone Apr 17, 2019
@blizzz

blizzz commented Apr 18, 2019

Copy link
Copy Markdown
Member

As LDAP seems to set the user name/email some more times, it basically added the 6 jobs on each login?

Hooks should not be fired if there was no change. But then, if every change lead to a new background job being created (and with the double-triggering you fixed on in another PR) 3 changes might be possible. Email, Displayname for sure, then perhaps avatar and/or quota.

rullzer
rullzer approved these changes May 9, 2019
View reviewed changes

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Didn't test in dept but makes sense

kesselb
kesselb reviewed May 9, 2019
View reviewed changes
Comment threadapps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php
@kesselb

kesselb commented May 9, 2019

Copy link
Copy Markdown
Contributor

Could you rebase? 🤔

@nickvergessen
nickvergessenforce-pushed the bugfix/noid/lookup-server-connector-background-jobs-problem branch from ff8dbf7 to a5c52bcCompareMay 9, 2019 13:28
@rullzer

rullzer commented May 11, 2019

Copy link
Copy Markdown
Member

@kesselb rebase happend. review?

@rullzer
rullzer requested a review from MorrisJobkeJuly 9, 2019 17:55
@MorrisJobkeMorrisJobke mentioned this pull request Jul 15, 2019
Merged
28 tasks
@nickvergessen

nickvergessen commented Jul 18, 2019

Copy link
Copy Markdown
MemberAuthor

Still waiting for reviews @MorrisJobke@blizzz@ChristophWurst

MorrisJobke
MorrisJobke approved these changes Jul 18, 2019
View reviewed changes

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke

MorrisJobke commented Jul 18, 2019

Copy link
Copy Markdown
Member

Let me rebase to trigger CI again.

@nickvergessen@MorrisJobke
Use an application class
3930b91
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen@MorrisJobke
Also send a delete like on the syncronous action
3a4f630
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen@MorrisJobke
Always use a background job for the update
9927909
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen@MorrisJobke
Use public interfaces
0061516
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen@MorrisJobke
Yay for null coalescing operator
e1e1d4d
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen@MorrisJobke
Make the RetryJob work on the userId only
30051e7
To ensure there is only 1 background job per user
Signed-off-by: Joas Schilling <coding@schilljs.com>
@MorrisJobke
MorrisJobkeforce-pushed the bugfix/noid/lookup-server-connector-background-jobs-problem branch from a5c52bc to 30051e7CompareJuly 18, 2019 09:24
@MorrisJobkeMorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 18, 2019
@MorrisJobke

MorrisJobke commented Jul 18, 2019

Copy link
Copy Markdown
Member

@nickvergessen Should we back port this?

@nickvergessen

nickvergessen commented Jul 18, 2019

Copy link
Copy Markdown
MemberAuthor

In theory it would be cool, but I think its too heavy :/

@MorrisJobke

MorrisJobke commented Jul 18, 2019

Copy link
Copy Markdown
Member

In theory it would be cool, but I think its too heavy :/

Okay. Then keeping it here.

@rullzer
rullzer merged commit 940a313 into masterJul 18, 2019
@delete-merged-branch
delete-merged-branchBot deleted the bugfix/noid/lookup-server-connector-background-jobs-problem branch July 18, 2019 11:10
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kesselbkesselbkesselb left review comments
@rullzerrullzerrullzer approved these changes
@MorrisJobkeMorrisJobkeMorrisJobke approved these changes
@ChristophWurstChristophWurstAwaiting requested review from ChristophWurst
@schiessleschiessleAwaiting requested review from schiessle
@blizzzblizzzAwaiting requested review from blizzz

Assignees

No one assigned

Labels

4. to releaseReady to be released and/or waiting for tests to finishbugfeature: federation

Projects

None yet

Milestone

Nextcloud 17

Development

Successfully merging this pull request may close these issues.

5 participants

@nickvergessen@blizzz@kesselb@rullzer@MorrisJobke