Skip to content

fix(caldav): automatically delete outdated scheduling objects - #45235

Merged
miaulalala merged 1 commit into
masterfrom
fix/remove-old-scheduling-objects
Jun 3, 2024
Merged

fix(caldav): automatically delete outdated scheduling objects#45235
miaulalala merged 1 commit into
masterfrom
fix/remove-old-scheduling-objects

Conversation

@miaulalala

@miaulalalamiaulalala commented May 8, 2024

Copy link
Copy Markdown
Contributor

Summary

oc_schedulingobjects currently grows without ever deleting outdated objects. This PR a repair step that is declared as expensive so admins can decide to run them at their convenience for the initial delete.

The delete is chunked to 50k rows on each transaction so the database isn't locked for a long time (especially in clustered setups this could cause issues). MySQL needs special treatment as it doesn't support LIMITs on DELETE queries, so it does a SELECT on the ids to delete, and then runs the delete on those.

After the repair step has run, a regular cron job is added to the Jobs List that runs every hour to get rid of scheduling objects that are older than an hour. We don't really need them and could theoretically delete them as soon as they're processed by the ITip\Broker but as rooms and resources are also run in a cron job, keeping them until the principal room and resources are added is probably a good idea as I can't exclude unwanted side effects. I also updated the runtime for rooms and resources to run every half hour for that reason.

Checklist

@miaulalalamiaulalala self-assigned this May 8, 2024
@miaulalalamiaulalala added 2. developing Work in progress performance 🚀 feature: caldav Related to CalDAV internals labels May 8, 2024
@miaulalalamiaulalala added this to the Nextcloud 30 milestone May 8, 2024
miaulalala

This comment was marked as outdated.

@miaulalala

This comment was marked as outdated.

Comment threadapps/dav/lib/BackgroundJob/DeleteOutdatedSchedulingObjects.php Outdated
@miaulalala

This comment was marked as outdated.

Comment threadapps/dav/lib/CalDAV/CalDavBackend.php Fixed
Comment threadlib/private/Log.php Fixed
Comment threadapps/dav/lib/Migration/DeleteSchedulingObjects.php Fixed
Comment threadapps/dav/lib/AppInfo/Application.php Fixed
@miaulalala
miaulalalaforce-pushed the fix/remove-old-scheduling-objects branch from cbcae93 to f2d7e9fCompareMay 16, 2024 18:22
@miaulalala
miaulalala marked this pull request as ready for review May 16, 2024 18:23
@miaulalala
miaulalala requested review from a team, Altahrim, nickvergessen, sorbaugh and yemkareems and removed request for a teamMay 16, 2024 18:23
@miaulalala
miaulalala marked this pull request as draft May 16, 2024 18:34

@ChristophWurstChristophWurst 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.

Let's get rid of the hourly full table scan

👍 otherwise

Comment threadapps/dav/lib/CalDAV/CalDavBackend.php Outdated
Comment threadapps/dav/lib/CalDAV/CalDavBackend.php Outdated
Comment threadapps/dav/lib/CalDAV/CalDavBackend.php Outdated
Comment threadapps/dav/lib/BackgroundJob/DeleteOutdatedSchedulingObjects.php Outdated
Comment threadapps/settings/lib/SetupChecks/SchedulingTableSize.php Fixed
Comment threadapps/dav/lib/Migration/Version1031Date20240523183435.php Outdated
->from('schedulingobjects')
->where($query->expr()->lt('lastmodified', $query->createNamedParameter($modifiedBefore)))
->setMaxResults($limit);
$result = $query->executeQuery();

Check notice

Code scanning / Psalm

MoreSpecificImplementedParamType

Argument 3 of OCA\DAV\CalDAV\CalDavBackend::createSchedulingObject has the more specific type 'string', expecting 'resource|string' as defined by Sabre\CalDAV\Backend\SchedulingSupport::createSchedulingObject
@miaulalala
miaulalala marked this pull request as ready for review May 28, 2024 15:55
@miaulalalamiaulalala added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 28, 2024
Comment threadapps/dav/lib/CalDAV/CalDavBackend.php

@ChristophWurstChristophWurst 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.

Code looks good

ITimeFactory $timeFactory,
) {
parent::__construct($timeFactory);
$this->setInterval(23 * 60 * 60);

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.

Why not 24? 😆

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@nickvergessen can explain 😉

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.

it would move back all the time, as the run time is not 0 seconds and it can be started with up to 14 minutes delay.
That would mean it would fall further back and back in the "maintenance time window" and eventually skip some day as current time + 24h is outside of maintenance window.
With 23h it will just be saver to run every day.

@miaulalala
miaulalalaforce-pushed the fix/remove-old-scheduling-objects branch from 1a53766 to 6ba43b3CompareMay 28, 2024 19:18
@miaulalala

Copy link
Copy Markdown
ContributorAuthor

/backport to stable29

@miaulalala

Copy link
Copy Markdown
ContributorAuthor

/backport to stable28

@miaulalala

Copy link
Copy Markdown
ContributorAuthor

/backport to stable27

Comment threadapps/dav/lib/Migration/DeleteSchedulingObjects.php
Comment threadapps/settings/lib/SetupChecks/SchedulingTableSize.php
Signed-off-by: Anna Larch <anna@nextcloud.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviewsfeature: caldavRelated to CalDAV internalsperformance 🚀

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove old scheduling objects from INBOX and oc_schedulingobjects via cron

6 participants

@miaulalala@nickvergessen@ChristophWurst@tcitworld@kesselb@github-advanced-security