@diosmosis not sure how this would prevent the original issue? Removing the check for isInvalidationsScheduledForSite seems like it might only invalidate more often?

AFAIK we had added it to prevent the same invalidation for the same report, site , date being there under circumstances many times. Like we had it sometimes the same invalidation 20 times or more even though only one would have been fine. So we were adding this check to only invalidate more entries, once all existing invalidations have been processed.

I'm not understanding yet how this would result in less invalidations?

I'm not understanding yet how this would result in less invalidations?

@tsteur The place invalidations were done is moved. We were doing it on every iteration of queueconsumer, now we do it only when switching sites. Which is why we don't need the check anymore.

👍 didn't notice it was moved into the if. Can we keep the if ($this->model->isInvalidationsScheduledForSite($idSiteToInvalidate)) { check though? That be great as I think it will help hugely to not end up invalidating the same reports way too often and end up with a huge backlog of archive invalidations.

Can we keep the if ($this->model->isInvalidationsScheduledForSite($idSiteToInvalidate)) { check though?

I'm not sure if this is a good idea, doing it before each site means once per core:archive run per site. So if there happened to be an existing invalidation, because a user invalidated a single period, we'd archive that period, and nothing else for that site until the site was encountered again. Which means today/yesterday/etc. would not be re-archived for potentially many hours.

I reckon that be fine because a manual invalidation is usually rare vs in most other cases there would be maybe already many invalidations in there and we would add the same invalidations for today again. Maybe we'd need to change the check and see if there are invalidations for today and if so, then not invalidate again. If invalidations are in there for previous day we could invalidate again. This should minimise the risk more often.

For the 4.0.1 release I'd prefer not to risk too much. Could we simply move the invalidation call into the if. Then have a separate PR for 4.1.0 with the other change (which we could test on demo and demo2 for a while etc)?

I reckon that be fine because a manual invalidation is usually rare vs in most other cases there would be maybe already many invalidations in there and we would add the same invalidations for today again. Maybe we'd need to change the check and see if there are invalidations for today and if so, then not invalidate again. If invalidations are in there for previous day we could invalidate again. This should minimise the risk more often.

Ok, sure.

Great, let me know when this is done. Then we can merge and release an update.

@tsteur the tests fail w/ that change, debugging currently

@tsteur pushed