@tsteur opened this Pull Request on April 29th 2020 Member

refs https://github.com/matomo-org/matomo/pull/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"

@tsteur commented on May 1st 2020 Member

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

@diosmosis commented on June 27th 2020 Member

@tsteur fixed the tests, not sure why the UI tests are failing, but it seems unrelated

@tsteur commented on June 29th 2020 Member

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.

@tsteur commented on June 29th 2020 Member

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 https://github.com/matomo-org/matomo/pull/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 https://github.com/matomo-org/matomo/pull/15887/commits/6c2fa2ab372b5f8c4530500b289281dc631b4207#diff-dcb3557abe94edd8776f637afdfb76b7R103

@diosmosis commented on June 29th 2020 Member

@tsteur sounds good, mind if I merge it?

@tsteur commented on June 29th 2020 Member

Go for it

On Mon, 29 Jun 2020, 16:58 diosmosis, <notifications@github.com> 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
or unsubscribe

@diosmosis commented on June 29th 2020 Member

4.x-dev build looks ok merged, just some minor things unrelated to this pr

@tsteur commented on June 29th 2020 Member

👍 awesome

This Pull Request was closed on June 29th 2020
Powered by GitHub Issue Mirror