Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

disable cron task when segment is deleted #18383

Merged
merged 27 commits into from Jan 21, 2022
Merged

disable cron task when segment is deleted #18383

merged 27 commits into from Jan 21, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Nov 26, 2021 •
edited

Description:

Fixes:#16152
disable cron task when segment is deleted

Review

@peterhashair peterhashair marked this pull request as ready for review November 28, 2021 20:31
@peterhashair peterhashair added the Needs Review PRs that need a code review label Nov 28, 2021
@peterhashair peterhashair linked an issue Nov 28, 2021 that may be closed by this pull request
When a custom dimension is deleted, disable cron archiving for all segments that use this custom dimension #16152
Closed
@peterhashair peterhashair added this to the 4.7.0 milestone Nov 28, 2021
@peterhashair peterhashair added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Nov 28, 2021
@peterhashair
Copy link
Contributor Author

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

sgiehl
sgiehl requested changes Nov 29, 2021
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your approach won't work.
Also this would only solve one part of the requirements described here: #16152 (comment)

core/CronArchive/QueueConsumer.php Outdated Show resolved Hide resolved
core/CronArchive/QueueConsumer.php Outdated Show resolved Hide resolved
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Nov 29, 2021
@peterhashair
Copy link
Contributor Author

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

@peterhashair peterhashair added the Needs Review PRs that need a code review label Nov 30, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

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

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Dec 8, 2021
sgiehl
sgiehl requested changes Dec 8, 2021
View reviewed changes
core/CronArchive/QueueConsumer.php Outdated Show resolved Hide resolved
core/CronArchive/QueueConsumer.php Outdated Show resolved Hide resolved
core/CronArchive/QueueConsumer.php Outdated Show resolved Hide resolved
core/CronArchive/QueueConsumer.php Outdated Show resolved Hide resolved
@peterhashair peterhashair removed the Stale The label used by the Close Stale Issues action label Dec 9, 2021
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Dec 21, 2021
Peter Zhang added 2 commits December 23, 2021 09:40
Revert "Update QueueConsumer.php"
35cf3ff 
This reverts commit e059f4b.
Update QueueConsumer.php
0352f06 
remove the right part
@sgiehl
Update core/CronArchive/QueueConsumer.php
0bec608 
Co-authored-by: Stefan Giehl <stefan@matomo.org>
@peterhashair
Copy link
Contributor Author

peterhashair commented Jan 14, 2022 •
edited

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

matomo/core/CronArchive/SegmentArchiving.php

Lines 83 to 104 in 8e1aabf

public function findSegmentForHash($hash, $idSite)
{
foreach ($this->getAllSegments() as $segment) {
if (!$this->isAutoArchivingEnabledFor($segment)
|| !self::isSegmentForSite($segment, $idSite)
) {
continue;
}
try {
$segmentObj = new Segment($segment['definition'], [$idSite]);
} catch (\Exception $ex) {
$this->logger->debug("Could not process segment {$segment['definition']} for site {$idSite}. Segment should not exist for the site, but does.");
continue;
}
if ($segmentObj->getHash() == $hash) {
return $segment;
}
}
return null;
}

Peter and others added 2 commits January 14, 2022 17:55
@sgiehl
Copy link
Member

sgiehl commented Jan 14, 2022

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

fix segment errors
74633bc 
fix segment errors
@peterhashair
Copy link
Contributor Author

peterhashair commented Jan 17, 2022 •
edited

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

image

image

@peterhashair peterhashair modified the milestones: 4.7.0, 4.8.0 Jan 18, 2022
@sgiehl
Copy link
Member

sgiehl commented Jan 18, 2022

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

Peter added 2 commits January 19, 2022 11:06
update tests
324685a 
update tests
update tests and words
ffd613f 
update tests and words
@peterhashair
Copy link
Contributor Author

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

sgiehl
sgiehl reviewed Jan 19, 2022
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment. If it's to much effort to change that, I guess it should also be good to merge it that way.

core/CronArchive.php Show resolved Hide resolved
@peterhashair
Copy link
Contributor Author

@sgiehl update the log info. 😀

sgiehl
sgiehl approved these changes Jan 21, 2022
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now.

@sgiehl sgiehl merged commit b277655 into 4.x-dev Jan 21, 2022
@sgiehl sgiehl deleted the m-16152-dimension-deleted-cron-task-remove branch January 21, 2022 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.8.0
Development

Successfully merging this pull request may close these issues.

When a custom dimension is deleted, disable cron archiving for all segments that use this custom dimension
2 participants
@peterhashair @sgiehl