Ignore overwrite subtable warning for summary rows for old data to avoid re-archiving for a single row #17891
Conversation
…r a single row most users do not look at.
|
Seems one test is failing due to this changes. Btw. is there any issue related to this? |
|
@sgiehl L3-126 |
|
@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 |
|
@sgiehl this is discussed in the L3 issue. |
|
@sgiehl actually, no that's the wrong L3 issue, it's L3-85. Sorry about that. |
|
Don't merge, doesn't quite work |
|
@diosmosis I'll remove the needs review label for now. Feel free to add it again, once you are finished with fixing the PR. |
|
@sgiehl latest change appears to have stopped the issue |
There was a problem hiding this comment.
Even though the code might be more cloud related, I guess it should be good to merge.
@diosmosis Can we maybe remove that check again somewhen in the future? Wondering if it might make sense to add that in the comment then, so we maybe remove it in half a year or so? Or we could add a test to remove isStartDateLaterThanCloud441DeployDate after a certain date.
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. |
|
Ok. 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. |
|
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. |
|
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 |
|
@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. |
justinvelluppillai
changed the title
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