@diosmosis opened this Pull Request on August 16th 2021 Member

Description:

… to avoid re-archiving for a single row most users do not look at.

Two bugs related to serialization of summary rows w/ subtables were fixed in 4.4.1. Old data will still have missing or broken subtable links, however, and we don't want to have to re-archive everything for a single row no one looks at. Especially for users that have this issue, since this only occurs on high volume setups. So instead, we're ignoring it here.

If a user wants this data, they can invalidate and rearchive a period to have it display.

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] 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
@sgiehl commented on August 16th 2021 Member

Seems one test is failing due to this changes.

Btw. is there any issue related to this?

@diosmosis commented on August 16th 2021 Member

@sgiehl L3-126

@sgiehl commented on August 16th 2021 Member

@diosmosis the code actually looks quite cloud related. Is that relevant for other installations as well? If so guess the date wouldn't be correct as we can't know when someone updated to 4.4.1

@diosmosis commented on August 16th 2021 Member

@sgiehl this is discussed in the L3 issue.

@diosmosis commented on August 16th 2021 Member

@sgiehl actually, no that's the wrong L3 issue, it's L3-85. Sorry about that.

@diosmosis commented on August 17th 2021 Member

Don't merge, doesn't quite work

@sgiehl commented on August 17th 2021 Member

@diosmosis I'll remove the needs review label for now. Feel free to add it again, once you are finished with fixing the PR.

@diosmosis commented on August 17th 2021 Member

@sgiehl latest change appears to have stopped the issue

@diosmosis commented on August 18th 2021 Member

@sgiehl

Can we maybe remove that check again somewhen in the future?

No, it can't be removed, unless people who are affected by the issue (who have to be high traffic enough to trigger ranking query limits) decide to rearchive all or most past data. Which is not desirable.

@sgiehl commented on August 18th 2021 Member

Ok. But wouldn't this then still be triggered on an instance that was
updated after the cloud deploy date?

@diosmosis commented on August 18th 2021 Member

But wouldn't this then still be triggered on an instance that was updated after the cloud deploy date?

No, we're using the period start date now. This was the change in the latest commit.

@sgiehl commented on August 19th 2021 Member

But shouldn't we use the release date then? Guess someone could have updated earlier than the deploy date?

@diosmosis commented on August 19th 2021 Member

@sgiehl

But shouldn't we use the release date then? Guess someone could have updated earlier than the deploy date?

They could have but given we have not seen any complaints in this regard from users and these issues appear to have been around for a long time, maybe years and only affects high volume users who are most likely a smaller portion of non-cloud or non-on-premise users, we're not overly concerned with them. It might be nice to only add this for cloud, but that is not possible without an event which would degrade performance significantly for such a core method.

Note: this is all in the L3 issue, please read the entire discussion there so we can avoid this constant back and forth and so I don't have to repeat myself.

@tsteur commented on August 19th 2021 Member

Was gonna merge @diosmosis but not sure if eg this test https://app.travis-ci.com/github/matomo-org/matomo/jobs/531901169#L860 is failing because of this PR? or maybe it's unrelated. Be good to check the tests and then merge 🎉 great to have this fix

@diosmosis commented on August 19th 2021 Member

@tsteur I think that might be random... it's strange since the number of blobs in the output above doesn't change. I'll change it for now.

This Pull Request was closed on August 20th 2021
Powered by GitHub Issue Mirror