-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(cardav): support result truncation for addressbook federation #50092
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
base: master
Are you sure you want to change the base?
Conversation
c7ee3ab
to
1cb4891
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to test this? Does this also work with clients or is it limited strictly to server-to-server synchronization?
The limit is set to 1 here. The ways to test this that I can think of
PS: In case of federation both instances need to be patched.
It should work for client sync too, But testing server to server is also needed to cover the whole PR |
2e2983a
to
c10a8cd
Compare
/backport to stable31 |
c10a8cd
to
ef45c01
Compare
Failing CI needs to be addressed and won't be resolved with rebases |
ef45c01
to
9dc7d13
Compare
Please fill in description :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works with my changes.
Signed-off-by: Hamza Mahjoubi <[email protected]>
Signed-off-by: Richard Steinmetz <[email protected]>
f095a40
to
15c7f34
Compare
…tion Signed-off-by: Hamza <[email protected]>
…tion Signed-off-by: Hamza <[email protected]>
…tion Signed-off-by: Hamza <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@@ -850,6 +852,8 @@ public function deleteCard($addressBookId, $cardUri) { | |||
* @return array | |||
*/ | |||
public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel, $limit = null) { | |||
$maxLimit = $this->config->getSystemValueInt('carddav_sync_request_truncation', 50000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why a system value and not app=dav?
|
Signed-off-by: Hamza <[email protected]>
Does it still run into OOM with the now lower limit? |
I have 224 contacts, setting it to 50 caused the OOM, I'm assuming memory is not being freed, I'm still looking into it. Edit: Idk if it was obvious from my comment 😅 but setting it to > 224 solved the OOM exception |
$result['added'] = []; | ||
return $result; | ||
} | ||
$lastID = $values[array_key_last($values)]['id']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$lastID = $values[array_key_last($values)]['id']; | |
$lastId = $values[array_key_last($values)]['id']; |
} | ||
$lastID = $values[array_key_last($values)]['id']; | ||
if (count($values) >= $limit) { | ||
$result['syncToken'] = 'init_' . $lastID . '_' . $currentToken; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$result['syncToken'] = 'init_' . $lastID . '_' . $currentToken; | |
$result['syncToken'] = 'init_' . $lastId . '_' . $currentToken; |
Hi, I did some more testing today and encountered a few minor issues. I'm not entirely sure if this is moving in the right direction, so I've pushed the changes to a different branch based on this one: #53551
Please note that there have been some changes to config.sample.php in the meantime. If time allows, please rebase and resolve the conflicts. |
Summary
This Pr enables truncation for addressbook sync between federated instances
How to Test
what to look for:
Checklist