Allow date ranges to be passed to core:invalidate-report-data #14450

katebutler · 2019-05-15T23:00:29Z

Fixes #14178

mattab · 2019-05-16T00:23:30Z

Quick feedback:

would have expected the line unset($result[4]); // remove 'range' period to be removed as well, so that when you don't specify --periods it will also invalidate all ranges overlapping the invalidated date.

mattab · 2019-05-16T00:26:56Z

can you add some more test cases in

matomo/tests/PHPUnit/Integration/DataAccess/ArchiveInvalidatorTest.php

Lines 465 to 481 in 1a76568

    
           // range period, one site, cascade = true 
        
           array( 
        
               array(1), 
        
               array('2015-01-02', '2015-03-05'), 
        
               'range', 
        
               null, 
        
               true, 
        
               array( 
        
                   '2015_01' => array( 
        
                       '1.2015-01-01.2015-01-10.5.done.VisitsSummary', 
        
                   ), 
        
                   '2015_03' => array( 
        
                       '1.2015-03-04.2015-03-05.5.done.VisitsSummary', 
        
                       '1.2015-03-05.2015-03-10.5.done3736b708e4d20cfc10610e816a1b2341.UserCountry', 
        
                   ), 
        
               ), 
        
           ),

maybe

katebutler · 2019-06-17T21:25:47Z

Am working on integration tests for invalidating all overlapping date ranges when "all" is passed.

katebutler · 2019-06-19T20:45:24Z

Ready for review/merge

diosmosis · 2019-06-25T09:21:51Z

tests/PHPUnit/Integration/DataAccess/ArchiveInvalidatorTest.php

+                        '1.2014-10-15.2014-10-20.5.done3736b708e4d20cfc10610e816a1b2341',
+                    ),
+                ),
+            ),


Can we change one of these tests to have $cascadeDown = true just to test that behavior?

They are actually all passing $cascadeDown = true but this doesn't do anything as the range gets split down into its individual dates before the cascading is applied. You can get a cascade-like behaviour though by passing --periods=all to the command.

I noticed it didn't do anything, but I think it's important for our coverage to be complete to test that it does nothing in the case of ranges. Otherwise there's a small section of the functionality that isn't tested.

OK, I've chnaged a few of the tests to $cascadeDown = false

diosmosis · 2019-06-25T09:56:16Z

core/Archive/ArchiveInvalidator.php

+                $allDates[] = $date;
+            }
+            $dates = $allDates;
+        }


If I'm getting this right (and I probably am not, the invalidation code always confuses me), the reason we check for all subperiods is because ranges can span multiple months? In this case could we do addMonth(1) or something to check less dates? I think this might result in slightly more readable SQL, though that may not be too important in the long run.

It's for catching smaller subperiods that lie within the month. If we just check one day each month we'll miss the ones that don't include that day.

Again, I could be seeing it incorrectly, but I was thinking of using date pairs like 2019-01-01,2019-01-31 so we'd check if there's any range affecting that whole month, rather than individual days.

I discussed this yesterday with @mattab and we have decided that although it would make the SQL easier to read and possibly be more efficient too, it's not worth investing the time as we don't expect that the query will be run very often.

No worries.

tsteur · 2019-07-17T04:30:53Z

@katebutler @mattab

just tested and debugged to see how it works. I might have actually misunderstood and not quite sure which behaviour is expected.

So it seems right now when using eg ./console core:invalidate-report-data --periods=range --dates=2018-01-02,2018-02-02 it will invalidate the range period for each individual date. So it would invalidate period=range,date=2018-01-02,2018-01-02;period=range,date=2018-01-03,2018-01-03;.... period=range,date=2018-02-02,2018-02-02 but I would have now maybe expected it invalidates the exact date range where period=range, date=2018-01-02,2018-02-02. So basically something like this code:

diff --git a/core/Archive/ArchiveInvalidator.php b/core/Archive/ArchiveInvalidator.php
index 1f03a07dda..f3dd0a68c2 100644
--- a/core/Archive/ArchiveInvalidator.php
+++ b/core/Archive/ArchiveInvalidator.php
@@ -232,11 +232,8 @@ class ArchiveInvalidator
 
         // Get the individual dates for a range, we need to check all of them
         if ($periodType === 'range') {
-            $allDates = array();
-            for ($date = $dates[0]; $date->getTimestamp() <= $dates[1]->getTimestamp(); $date = $date->addDay(1)) {
-                $allDates[] = $date;
-            }
-            $dates = $allDates;
+            $periodsToInvalidate[] = Period\Factory::build($periodType, $dates[0]->toString() . ',' . $dates[1]->toString());
+            return $periodsToInvalidate;
         }
 
         foreach ($dates as $date) {

@mattab not quite sure what is supposed to happen?

tsteur · 2019-07-29T02:42:36Z

@mattab still need your input here

mattab · 2019-08-09T02:45:17Z

@tsteur i would have also expected what you propose... @katebutler do you maybe remember the idea behind the current PR implementation?

…validate all archived ranges that overlap the argument range, else we invalidate exact match only

katebutler · 2019-08-26T21:55:33Z

Logic has been re-implemented so that:

If the range period type is passed, only archives whose date ranges exactly match the input range(s) will be invalidated. This should be more intuitive to the user, as we are only invalidating exactly what we've been asked to.
If the all period type is passed, all archives whose date ranges overlap the input range(s) will be invalidated. This is the correct behaviour when the user has recently removed some data (e.g. for GDPR compliance) and needs all archives that may contain that data to be invalidated.

tsteur · 2019-10-10T20:47:47Z

core/DataAccess/Model.php

+        foreach ($ranges as $range) {
+            // Ranges in the DB match if their date2 is after the start of the search range and date2 is before the end
+            $dateConditions[] = "(date2 >= ? AND date1 <= ?)";
+            $bind[] = $range->getDateStart()->toString();


When I execute ./console core:invalidate-report-data --dates=2019-02-03,2019-02-04 I get an error

Uncaught exception: Error: Call to a member function getDateStart() on array in core/DataAccess/Model.php:178

Also when using ./console core:invalidate-report-data --dates=2019-02-03,2019-02-10 --periods=all

tsteur · 2019-10-14T02:27:19Z

core/DataAccess/Model.php

+
+        foreach ($ranges as $range) {
+            // Ranges in the DB match if their date2 is after the start of the search range and date2 is before the end
+            $dateConditions[] = "(date2 >= ? AND date1 <= ?)";


@katebutler not sure if I'm understanding this correctly.

I have here this query generated:

UPDATE piwik_archive_numeric_2012_04 SET value = 4 WHERE name LIKE 'done%' AND idsite IN (1) AND (period = 5 AND ((date2 >= ? AND date1 <= ?)))

and the binds are ['2019-02-03', '2019-02-04']. Does date1 and date2 maybe need to be switched? The comment mentions twice date2 so that's also not quite clear. I called it like this:

./console core:invalidate-report-data --dates=2019-02-03,2019-02-04 --periods=range

Then I would expect date ranges like these to be updated:

2019-01-04,2019-03-03 2019-02-03,2019-03-03 2019-01-04,2019-02-03 2019-01-04,2019-02-04

but with this query I don't think this works? I might be seeing it wrong though. Or maybe we want it to behave differently? @mattab ?

tsteur · 2019-10-14T02:34:18Z

core/DataAccess/Model.php

+     * @return \Zend_Db_Statement
+     * @throws Exception
+     */
+    public function updateAllRangeArchivesAsInvalidated($archiveTable, $idSites, $ranges, Segment $segment = null)


Looking at the method updateArchiveAsInvalidated https://github.com/matomo-org/matomo/blob/3.x-dev/core/DataAccess/Model.php#L108-L119 I think this is actually updating it as expected using (date1 <= ? AND ? <= date2)? I wonder is this method even needed? Not sure how it behaves different to the other method?

Actually realising there is a new if in the mentioned method that only deletes an exact range when --periods=range is set. Not sure we want that behaviour. I reckon we can remove that if and restore old behaviour and then we can also use the updateArchiveAsInvalidated method again. I reckon it might be fine to use same behaviour for when using --periods=range and when using --periods=all. Currently it seems to be using different behaviour when using --periods=range where it only deletes the exact given range but it should maybe delete all ranges it is present.

ping @mattab not exactly sure what behaviour we want

using --periods=all. Currently it seems to be using different behaviour when using --periods=range where it only deletes the exact given range but it should maybe delete all ranges it is present.

👍 when --periods=all then all ranges should be invalidated.

tsteur · 2019-10-14T02:41:13Z

core/DataAccess/Model.php

-                $dateConditions[] = "(date1 <= ? AND ? <= date2)";
-                $bind[] = $date;
-                $bind[] = $date;
+            if ($periodType == Period\Range::PERIOD_ID) {


This is the if I mean below. I reckon we could restore old behaviour and delete all ranges it affects when using --period=range. I understand when using --period=range the problem is that it wouldn't iterate over all archive tables. For this we could always call markArchivesOverlappingRangeAsInvalidated() for the ranges and instead of updateAllRangeArchivesAsInvalidated call updateArchiveAsInvalidated.

tsteur · 2019-10-14T02:42:04Z

@katebutler lets discuss details tmrw. Also waiting to here from @mattab the comments. I reckon though it be good to let it behave the same way when --periods=range is used and when --periods=all. We don't really want it to behave differently.

tsteur · 2019-10-14T20:24:38Z

It's hard to explain what I meant so quickly made 3-4 changes... lets discuss later and see what makes sense... I meant something like this:

https://github.com/matomo-org/matomo/compare/14178_2?expand=1

see my last commit df13cbe where we basically handle period range separately. (didn't test it at all)... there are still some things that would need to be adjusted and restored maybe such as https://github.com/matomo-org/matomo/compare/14178_2?expand=1#diff-27aba6f51c03fb57f660eeb0b2a18484L234 maybe... seems to be making the whole PR easier though

…ods=all --dates=2019-01-02

Allow date ranges to be passed to core:invalidate-report-data

aa5f373

katebutler added the Needs Review PRs that need a code review label May 15, 2019

katebutler added this to the 3.11.0 milestone May 15, 2019

Unit tests for invalidating date ranges

d24512d

katebutler added 2 commits June 20, 2019 05:38

Include range periods in invalidate 'all'

e48252d

Merge branch '3.x-dev' into 14178

477bbae

diosmosis reviewed Jun 25, 2019

View reviewed changes

Unit tests for cascadeDown with range period

a72ec09

mattab modified the milestones: 3.11.0, 3.12.0 Jul 23, 2019

katebutler added 3 commits August 23, 2019 15:41

New archive invalidation logic for date ranges: if "all" passed we in…

3da5dc6

…validate all archived ranges that overlap the argument range, else we invalidate exact match only

Redo archive invalidation for date ranges

4ac12dc

Use new date range invalidation logic from InvalidateReportData command

5019ba0

tsteur requested changes Oct 10, 2019

View reviewed changes

katebutler added 2 commits October 14, 2019 11:39

Merge remote-tracking branch 'origin/3.x-dev' into 14178

77db710

Fix Exception

c9bb6f5

tsteur reviewed Oct 14, 2019

View reviewed changes

katebutler and others added 5 commits October 16, 2019 09:52

Invalidate all overlapping ranges when periodType=range

8359602

Invalidate all overlapping ranges when periodType=range

c8a942b

do not process range twice when using --periods=range

037c28b

fix exception when using ./console core:invalidate-report-data --peri…

9da6cf9

…ods=all --dates=2019-01-02

fix and add some tests

8df8253

tsteur merged commit 8b2964a into 3.x-dev Oct 16, 2019

tsteur deleted the 14178 branch October 16, 2019 01:01

mattab mentioned this pull request Jan 23, 2020

CoreAdminHome.invalidateArchivedReports says it supports range dates in doc but doesn't really support it #9898

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow date ranges to be passed to core:invalidate-report-data #14450

Allow date ranges to be passed to core:invalidate-report-data #14450

katebutler commented May 15, 2019

mattab commented May 16, 2019

mattab commented May 16, 2019

katebutler commented Jun 17, 2019

katebutler commented Jun 19, 2019

diosmosis Jun 25, 2019

katebutler Jun 27, 2019

diosmosis Jun 27, 2019

katebutler Jul 1, 2019

diosmosis Jun 25, 2019

katebutler Jun 27, 2019

diosmosis Jun 27, 2019

katebutler Jul 1, 2019

diosmosis Jul 1, 2019

tsteur commented Jul 17, 2019

tsteur commented Jul 29, 2019

mattab commented Aug 9, 2019

katebutler commented Aug 26, 2019

tsteur Oct 10, 2019

tsteur Oct 10, 2019

tsteur Oct 14, 2019 •

edited

tsteur Oct 14, 2019

tsteur Oct 14, 2019

tsteur Oct 14, 2019

mattab Oct 14, 2019

tsteur Oct 14, 2019

tsteur commented Oct 14, 2019

tsteur commented Oct 14, 2019

Allow date ranges to be passed to core:invalidate-report-data #14450

Allow date ranges to be passed to core:invalidate-report-data #14450

Conversation

katebutler commented May 15, 2019

mattab commented May 16, 2019

mattab commented May 16, 2019

katebutler commented Jun 17, 2019

katebutler commented Jun 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur commented Jul 17, 2019

tsteur commented Jul 29, 2019

mattab commented Aug 9, 2019

katebutler commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur Oct 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur commented Oct 14, 2019

tsteur commented Oct 14, 2019

tsteur Oct 14, 2019 •

edited