@diosmosis opened this Pull Request on July 6th 2019 Member

CC @tsteur

Seems like it works fine, tested manually. Automated tests shouldn't pass just yet, will work on that soon.

Refs #14091

@diosmosis commented on July 8th 2019 Member

NOTE: this will require a change to CustomReports. Actually I'll edit to maintain BC.

@tsteur commented on July 11th 2019 Member

@diosmosis sounds good to change custom reports as long as we can still support previous Matomo versions as well.

@diosmosis commented on July 12th 2019 Member

@tsteur changes should be BC now. However, I noticed AbTesting does some stuff w/ ArchiveWriter that includes a temporary archive (in GetMetricsOverview.php). Will this be a problem? Not sure what the code does.

@tsteur commented on July 14th 2019 Member

Looks promising so far @diosmosis . Should we maybe merge this with PR https://github.com/matomo-org/matomo/pull/14091 as we maybe still need the changes in CronArchive?

@diosmosis commented on July 16th 2019 Member

Should we maybe merge this with PR #14091 as we maybe still need the changes in CronArchive?

@tsteur I'm not sure that PR as it is done is even needed anymore... If we only run archiving for invalidated/missing archives, then we don't even have to check for visits in log_visit. So instead of every other check, just see if there is an idarchive w/ DONE_OK for the site/period/segment/etc., and if not send the request. Otherwise, skip it. What do you think? Since we invalidate on tracking, I think this should work, ie, if there are no visits, the archive won't be invalidated.

@tsteur commented on July 16th 2019 Member

I meant eg in CronArchiving, before issuing an archive request in $cronArchiver->request($url) or so, we need to check if we actually need to archive any data. This will safe a lot of time / performance in the archiver. Eg if data has been invalidated recently, we will need to issue the request... if a completed archive is there, and no data has been invalidated, then we don't need to issue the archiving request.

@diosmosis commented on July 16th 2019 Member

@tsteur Yes that's what I meant, but the previous PR checks if there are visits in log_visit. Here I'm suggesting just to check if there's an existing archive w/ value = DONE_OK. Which might be what you're suggesting as well? The plan was to do that in another PR.

@tsteur commented on July 16th 2019 Member

Sounds good to do this in another plan, I'll review the PR again later this week then. 👍 Looks good so far though had a look already a few times.

@tsteur commented on July 17th 2019 Member

@diosmosis can you look at the 2 failing tests https://travis-ci.org/matomo-org/matomo/jobs/557621873#L829-L844 ? otherwise looks good to merge and work on the CronArchiver in another PR.

Also left a comment where we possibly break the API and need to put back a parameter. One print debug is also there, otherwise feel free to merge once these things are done .

This Pull Request was closed on August 6th 2019
Powered by GitHub Issue Mirror