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(dav): Add retention time to sync token cleanup #44075

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Mar 8, 2024

Summary

This is a refinement of #31064. We keep 10k sync changes by default. The larger the user base, the fewer sync tokens can be stored per user on average. E.g. if you have an installation of 5k active calendar users, each of them can only keep 2 sync changes. This leads to synchronization problems with Thunderbird and similar.
This change adds timestamp tracking to changes and only deletes changes older than 60 days. The retention period is configurable. 7 days is the minimum.

TODO

  • ...

Checklist

@ChristophWurst ChristophWurst added bug 2. developing Work in progress feature: dav feature: caldav Related to CalDAV internals feature: carddav Related to CardDAV internals labels Mar 8, 2024
@ChristophWurst ChristophWurst self-assigned this Mar 8, 2024
@ChristophWurst
Copy link
Member Author

@tcitworld does this makes sense?

Copy link
Member

@tcitworld tcitworld left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -52,9 +52,10 @@ public function __construct(ITimeFactory $timeFactory, CalDavBackend $calDavBack

public function run($argument) {
$limit = max(1, (int) $this->config->getAppValue(Application::APP_ID, 'totalNumberOfSyncTokensToKeep', '10000'));
$retention = max(7, (int) $this->config->getAppValue(Application::APP_ID, 'syncTokensRetention', '60')) * 24 * 3600;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$retention = max(7, (int) $this->config->getAppValue(Application::APP_ID, 'syncTokensRetention', '60')) * 24 * 3600;
$retention = max(7, (int) $this->config->getAppValue(Application::APP_ID, 'syncTokensRetentionDays', '60')) * 24 * 3600;

Make the name explicit the unit

apps/dav/lib/BackgroundJob/PruneOutdatedSyncTokensJob.php Outdated Show resolved Hide resolved
@ChristophWurst
Copy link
Member Author

/backport to stable28

@ChristophWurst
Copy link
Member Author

/backport to stable27

@ChristophWurst
Copy link
Member Author

/backport to stable26

@@ -52,9 +52,10 @@

public function run($argument) {
$limit = max(1, (int) $this->config->getAppValue(Application::APP_ID, 'totalNumberOfSyncTokensToKeep', '10000'));
$retention = max(7, (int) $this->config->getAppValue(Application::APP_ID, 'syncTokensRetentionDays', '60')) * 24 * 3600;

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\IConfig::getAppValue has been marked as deprecated
Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, Psalmy. I want to backport

Copy link
Member

Choose a reason for hiding this comment

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

backport ease vs. future tech debt
⚖️
The never ending balancing act...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll send a PR when this is in

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 11, 2024
@ChristophWurst ChristophWurst marked this pull request as ready for review March 11, 2024 08:42
Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

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

LGTM

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 11, 2024
@ChristophWurst ChristophWurst force-pushed the fix/dav/sync-token-retention-time branch from 3054011 to 4ddd76c Compare March 12, 2024 14:09
@ChristophWurst
Copy link
Member Author

CardDavBackendTest::testPruneOutdatedSyncTokens fails

@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Mar 12, 2024
@ChristophWurst ChristophWurst mentioned this pull request Mar 21, 2024
@ChristophWurst ChristophWurst force-pushed the fix/dav/sync-token-retention-time branch from 3d68b2d to f3e295f Compare March 21, 2024 09:20
@ChristophWurst
Copy link
Member Author

Fixed existing tests and added two new assertions that assert all data is kept when the retention threshold is timestamp=0.

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Mar 21, 2024
@Altahrim
Copy link
Collaborator

There is still some errors in CI

@ChristophWurst ChristophWurst force-pushed the fix/dav/sync-token-retention-time branch from f3e295f to 77e2c60 Compare March 21, 2024 15:47
@ChristophWurst
Copy link
Member Author

Addressed. Now CardDAV has the same new test to assert the logic keeps historic data :)

@ChristophWurst ChristophWurst force-pushed the fix/dav/sync-token-retention-time branch 2 times, most recently from f84412a to dd35045 Compare March 21, 2024 16:37
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish backport-request bug feature: caldav Related to CalDAV internals feature: carddav Related to CardDAV internals feature: dav
Projects
Development

Successfully merging this pull request may close these issues.

8 participants