@diosmosis opened this Pull Request on November 25th 2020 Member

…idate again until a core:archive run sees that site.

Description:

Fixes #16788

Changing from invalidating on every iteration if there are no existing invalidations to invalidating once per site no matter if there are invalidations already. The idea for the previous one was to not have to wait for hours for a particularly big site to finish to process another user provided invalidation, but this would be the case anyway if the site had finished.

CC @tsteur

Review

  • [ ] Functional review done
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@tsteur commented on November 25th 2020 Member

@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?

@diosmosis commented on November 25th 2020 Member

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.

@tsteur commented on November 25th 2020 Member

👍 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.

@diosmosis commented on November 25th 2020 Member

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.

@tsteur commented on November 25th 2020 Member

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)?

@diosmosis commented on November 25th 2020 Member

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.

@tsteur commented on November 26th 2020 Member

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

@diosmosis commented on November 26th 2020 Member

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

@diosmosis commented on November 26th 2020 Member

@tsteur pushed

This Pull Request was closed on November 26th 2020
Powered by GitHub Issue Mirror