@peterhashair opened this Pull Request on November 26th 2021 Contributor

Description:

Fixes:#16152
disable cron task when segment is deleted

Review

@peterhashair commented on November 28th 2021 Contributor

not sure that's the right solution. but it should skip the segment mark as deleted.

@peterhashair commented on November 30th 2021 Contributor

@sgiehl right, that makes more sense now. Update a little bit, hopeful I am on the right direction :)

@github-actions[bot] commented on December 8th 2021 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@peterhashair commented on January 14th 2022 Contributor

@sgiehl Have a deep look at this one, if I am right I think a new function doesn't even needed, it seems like already checked in here. I guess it's somewhere else.

https://github.com/matomo-org/matomo/blob/8e1aabfc68ad16ea8af059a9108a54e6053aa414/core/CronArchive/SegmentArchiving.php#L83-L104

@sgiehl commented on January 14th 2022 Member

@peterhashair Were you actually able to reproduce the error that is described in the issue in any way?
The only way I was able to reproduce the exact same error message was when I disabled a plugin while archiving was running. So the archiving requests then failed due to the missing segments. If the segment is already invalid before the archiving starts it seems to be skipped without resulting in an error.

I have created a test case, that reflects the way I was able to reproduce. So the process that starts the archiving receives a segment from UserLanguage plugin. But for the archiving requests that are sent the plugin is disabled, causing the segment to be invalid.
The test currently fails. I guess the target would be to adjust the archiving, so such requests with an invalid segment won't throw an error or that specific error is handled properly, without resulting in a failure.

If you were able to reproduce that another way, feel free to add an additional test case for that as well.

@peterhashair commented on January 17th 2022 Contributor

@sgiehl I saw the new test, reproduced it. Should be fixed now.

image

image

@sgiehl commented on January 18th 2022 Member

@peterhashair Actually you have adjusted the test case so it covers another problem. Disabling the plugin before the archiver runs, let's the archiver skip the segments. That actually was already the case before, even though the message was not that clear.
But the error message of the original issue is

ERROR [2018-03-12 00:12:06] Got invalid response from API request: ?module=API&method=API.get&idSite=2&period=day&date=last52&format=php&trigger=archivephp. Response was 'a:2:{s:6:"result";s:5:"error";s:7:"message";s:81:"Segment 'dimension7' is not a supported segment. - caused by plugin CustomReports";}'

That actually happens if the archiver already started, but while looping over the archiving requests the plugin gets disabled. So a request for archiving an invalid segment is sent, causing the error above.

I guess we need to ignore if a archiving request results in that specific error. And log an info about that instead.

@peterhashair commented on January 19th 2022 Contributor

@sgiehl right, that makes more sense. Update a little. Hopefully is correct :)

@peterhashair commented on January 20th 2022 Contributor

@sgiehl update the log info. 😀

This Pull Request was closed on January 21st 2022
Powered by GitHub Issue Mirror