More rigorous checks for non-existant site so archiving wont fail if … #16837
Conversation
…a site is deleted + tweaks to some debug logs
| try { | ||
| $this->assertEquals($expectedInvalidationsFound, $iteratedInvalidations); | ||
| } catch (\Exception $ex) { | ||
| print "\nInvalidations inserted:\n" . var_export($invalidations, true) . "\n"; | ||
| throw $ex; | ||
| } |
There was a problem hiding this comment.
Why should assertEquals throw an exception? Shouldn't that be more like
| try { | |
| $this->assertEquals($expectedInvalidationsFound, $iteratedInvalidations); | |
| } catch (\Exception $ex) { | |
| print "\nInvalidations inserted:\n" . var_export($invalidations, true) . "\n"; | |
| throw $ex; | |
| } | |
| $this->assertEquals($expectedInvalidationsFound, $iteratedInvalidations, "Invalidations inserted:\n" . var_export($invalidations, true)); |
There was a problem hiding this comment.
As long as the message (which will be very long) is readable and only displayed on failure, that would work too.
| } | ||
|
|
||
| foreach ($next as &$item) { | ||
| Db::query("UPDATE " . Common::prefixTable('archive_invalidations') . " SET status = 1 WHERE idinvalidation = ?", [$item['idinvalidation']]); |
There was a problem hiding this comment.
Not sure what that update is good for. Is it so getNextArchivesToProcess would not end up in a loop if any record is returned? 🤔
There was a problem hiding this comment.
QueueConsumer only iterates through invalidations, it does not start archives, CronArchive does this as it launches new processes. This is to simulate this for the test, since QueueConsumer is only part of the whole loop.
|
|
||
| private function getInvalidationCount() | ||
| { | ||
| return Db::fetchOne("SELECT COUNT(*) FROM " . Common::prefixTable('archive_invalidations')); |
There was a problem hiding this comment.
That method actually isn't used anywhere 🤔
core/CronArchive.php
Outdated
| private function siteExists($idSite) | ||
| { | ||
| try { | ||
| $site = APISitesManager::getInstance()->getSiteFromId($idSite); |
There was a problem hiding this comment.
btw could use new Site($idSite) here directly as it would use cache but maybe that be actually the problem and we don't want to hit the cache.
There was a problem hiding this comment.
actually be great if this hit the cache maybe? or would it cause a problem if the site was suddenly removed?
There was a problem hiding this comment.
The cache would exist for the entire core:archive run, so a site could be removed say 30 mins after archiving starts, but it's data could still be cached, and we'd try to process it. We could maybe clear the cache before starting a new site archive so it's cached a bit... I'll think about it.
|
Looks generally good once tests pass @diosmosis 👍 there are a few comments to look at and a merge conflict but nothing major. |
…a site is deleted + tweaks to some debug logs
Description:
Deleted some sites while a canceled/failed archive had been run before, and the next core:archive run would keep failing since the sites in the list didn't exist. Added some more checks for whether the site exists or not and fixed the existing check method.
FYI @tsteur
Review