Improve performance/memory of archive selection #15887
Conversation
|
I can kind of get the tests to work but I'm quite certain there is a bug in the current archive selection. Might need to do some call some day to understand things why the current implementation is. I'm not 100% sure but I think I can see that we falsely assume a site might have an invalidated archive when there is not (since we partially except any done flag)... leading potentially to unneeded archive invalidations maybe... And we partially use possibly maybe the wrong visits/visits converted values in circumstances although this one might be fine. Maybe I come back to this at some point but might just close it as it's hard to really understand how it is supposed to work and not to work |
|
@tsteur fixed the tests, not sure why the UI tests are failing, but it seems unrelated |
There was a problem hiding this comment.
Left a short comment @diosmosis
Generally looks good to merge just trying to understand the other comment where we have no done flag...
We'll merge this then into 4.x, not 3.x. I would say.
| @@ -172,7 +172,7 @@ public function getTestDataForGetArchiveIdAndVisits() | |||
| '', | |||
| $minDateProcessed, | |||
| false, | |||
| [false, false, false, true], | |||
| [false, false, false, false], | |||
There was a problem hiding this comment.
@diosmosis I suppose this is why we create now more archives below in eg https://github.com/matomo-org/matomo/pull/15887/files#diff-dcb3557abe94edd8776f637afdfb76b7R99
Do you know if this was a bug before that we would have assumed there was an archive when there was no done flag? Trying to understand how this used to work. Maybe in the past at the beginning of Matomo there were no done flags? No idea...
There was a problem hiding this comment.
I think before we would use any number of visits even if it wasn't the latest archive. So if there was:
- an archive w/ ts_archived = 14:23, no nb_visits
- an archive w/ ts_archived = 02:55, nb_visits = 5
We'd say there were 5 visits. Which means if in between if someone deleted all the visits it would still think there are 5. so probably a bug.
|
fyi @diosmosis changed the base to 4.x-dev and fixed the merge conflicts so will see what the tests do... there were 2 conflicts in tests so I expect to see some failures there where likely only need to adjust the fixtures. |
|
Thanks for triggering the build @diosmosis didn't realise it wasn't automatically running another built when changing the base to 4.x-dev. It's doing the funny thing again where it can't run the builds... same that happened in the other build that was updating the consent page. And just like in the other PR all tests pass in the PUSH build even though we'd probably expect them to fail. In #15973 the PUSH build was also green but it couldn't have been green because the page was changed. Once merged, the failure was visible in the 4.x-dev branch. So I reckon we might need to merge this PR, and then we can probably some new test failures in 4.x-dev branch from the merge commit in 6c2fa2a#diff-dcb3557abe94edd8776f637afdfb76b7R103 |
|
@tsteur sounds good, mind if I merge it? |
|
Go for it
…On Mon, 29 Jun 2020, 16:58 diosmosis, ***@***.***> wrote:
@tsteur <https://github.com/tsteur> sounds good, mind if I merge it?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#15887 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACCVYEZAOV7CARW77M5VETRZANQTANCNFSM4MUHLN5Q>
.
|
|
4.x-dev build looks ok merged, just some minor things unrelated to this pr |
|
👍 awesome |
refs #15879
It's a very minor improvement in terms of performance or memory but figured this is executed every time we fetch a report so may be better to directly fetch the right result and be easier to debug. While debugging some archiving memory issues noticed this query returns often 3000 results (depends on the number of segments etc... can be more or less). It's mostly saving PHP time and less time in "sending data"