-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Introduce single user and seen users sync #30485
Conversation
1cf0296
to
96df587
Compare
Codecov Report
@@ Coverage Diff @@
## master #30485 +/- ##
============================================
+ Coverage 62.29% 62.46% +0.16%
- Complexity 18208 18232 +24
============================================
Files 1142 1145 +3
Lines 68210 68289 +79
Branches 1234 1234
============================================
+ Hits 42494 42655 +161
+ Misses 25355 25273 -82
Partials 361 361
Continue to review full report at Codecov.
|
Blocked by #30412 |
I will rebase |
2580334
to
0730372
Compare
Rebased and squashed |
@ownclouders rebase |
Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently |
Automated rebase with GitMate.io was successful! 🎉 |
We need to add unit tests for the iterators and the accountMapper findByUserIds |
0730372
to
3615f08
Compare
3615f08
to
f65b9bb
Compare
Rebased |
f65b9bb
to
e671cd0
Compare
rebase attempt #4 ...... |
@ownclouders rebase |
Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently |
Automated rebase with GitMate.io was successful! 🎉 |
e671cd0
to
07a4bf9
Compare
tests pass now, only codecov is unhappy |
@ownclouders rebase please |
Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently |
codecov was showing weird results |
Automated rebase with GitMate.io was successful! 🎉 |
07a4bf9
to
d1f84d6
Compare
@tomneedham please review the last additions then merge |
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.
I tried this command already, and can confirm it worked properly, at least some time ago. Iterator seemed to work properly, and all tests are there. I just have one doubt in the query itself as mentioned below
$qb = $this->db->getQueryBuilder(); | ||
$qb->select('user_id') | ||
->from($this->getTableName()) | ||
->orderBy('user_id'); // needed for predictable limit & offset |
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.
👍
if ($hasLoggedIn === true) { | ||
$qb->andWhere($qb->expr()->gt('last_login', new Literal(0))); | ||
} else if ($hasLoggedIn === false) { | ||
$qb->andWhere($qb->expr()->eq('last_login', new Literal(0))); |
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.
Is all fine here? In both cases you have Literal(0) @butonic
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.
ohh, sorry just noticed gt and eq.
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Restart of #28212, but without additional logging. Based on #30412
We had to add a
findUserIds
method to the Account mapper to iterate over all users in batches.callForSeenUsers
takes a callback but also initializes the user objects filling caches everywhere, an unwanted side effect that only slows down the sync.We also introduced Iterators for Iterating over all users of a backend as well as seen users only. The case for a single user uses an ArrayIterator.
One change in behavior: to get a total count you now have to use
--showCount
. Counting might be expensive. So we disable it by default ...Tasks for another PR: