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

Do not run the same archive command twice #12702

Merged
merged 10 commits into from Apr 23, 2018
Merged

Do not run the same archive command twice #12702

merged 10 commits into from Apr 23, 2018

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Apr 8, 2018

Currently, the cron archiver might start the same archiving command twice. For example when a site takes long to archive. To avoid running the same archiving process in parallel, we should check if such a job is already running and if so, not start the same archive command again.

I did some simple testing on my local machine but might need some more testing.

Only supports CLI archiving, not web archiving.

@tsteur tsteur added the Needs Review PRs that need a code review label Apr 8, 2018
@tsteur tsteur added this to the 3.4.1 milestone Apr 8, 2018
@tsteur
Copy link
Member Author

tsteur commented Apr 9, 2018

FYI: https://github.com/matomo-org/matomo/blob/archivenottwice/core/CronArchive.php#L705-L716 might cause some trouble in that possibly a weekly archive will be skipped - because the weekly is already in process but not finished - so it already starts the monthly. However, the weekly is not finished. This has been a problem for a while when eg a weekly archive fails but think might be even more of a problem now.

@mattab
Copy link
Member

mattab commented Apr 9, 2018

@tsteur Good point, this PR should fix this issue? #12708

…te, abort the website archiving (#12708)

* If one of the period archiving fails, we now cancel the remaining archiving processes
* Increment the skipped counter
@mattab
Copy link
Member

mattab commented Apr 12, 2018

I found another use case where one can trigger the same job in parallel twice, when --force-idsites=X,Y,... is used. It could be that someone starts archiving with the force idsites option, and then core:archive would also start again the same archiving. Or it could be the other way around.
-> So maybe we could check for processes core:archive that use --force-idsites with the siteid we're about to process and skip if it is.

(For example I just had an issue while re-processing some data after it was manually invalidated, and the core:archive started again to run while my force-idsites was running)


$periodInProgress = $this->isAlreadyArchivingAnyLowerPeriod($idSite, $period);
if ($periodInProgress) {
$this->logger->info("- skipping archiving for period '{period}' because processing the period '{periodcheck}' is already in progress.", array('period' => $period, 'periodcheck' => $periodInProgress));
Copy link
Member Author

@tsteur tsteur Apr 12, 2018

Choose a reason for hiding this comment

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

fyi @mattab saw it in logs

skipping archiving for period 'year' because processing the period 'day' is already in progress.

@mattab mattab merged commit f0ced61 into 3.x-dev Apr 23, 2018
@mattab mattab deleted the archivenottwice branch April 23, 2018 02:01
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
* Do not run the same archive command twice

* simplify

*  During core:archive when a day or period archiving fails for a website, abort the website archiving (matomo-org#12708)

* If one of the period archiving fails, we now cancel the remaining archiving processes
* Increment the skipped counter

* also exit when a day period is already active

* undo last commit

* if day archive fails, then do not proceed archiving periods

* Minor changes

* Archive: mark the daily archive as successful, only after it was really successful (matomo-org#12720)

+ Simplify & fix some logic

* do not archive a period when a sub period is processing (matomo-org#12719)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants