Start storing the processing host and process id for started invalidations #22981
Conversation
sgiehl
force-pushed
the
dev-18888
branch
2 times, most recently
from
64caadf to
7131058
Compare
|
The storing part looks ok. Do we need to clean up the values at some point or they will remain in the table for the given invalidation? Can it happen an invalidation will be picked up multiple times and we will rewrite the data with the latest processing info, possibly losing the previous one? And if so, is that a problem? |
|
@michalkleiner would you mind to stop reviewing and changing unfinished PRs please? This has not yet been put into review |
|
Apologies @sgiehl 🙇 |
| $archiveFailureRecoveryTimeout = GeneralConfig::getConfigValue('archive_failure_recovery_timeout', $invalidation['idsite']); | ||
| if ( | ||
| empty($invalidation['ts_started']) | ||
| || $invalidation['ts_started'] > Date::now()->subSeconds($archiveFailureRecoveryTimeout)->getTimestamp() |
There was a problem hiding this comment.
This check never worked properly as ts_archived is provided from the database as datetime like 2025-01-01 12:00:00. Comparing that this way with a timestamp always results in true, causing the code below the check to never get executed.
As we are resetting stale invalidations in the begin of the archiving process, that shouldn't be required here anyway.
There was a problem hiding this comment.
Tested manually and it works as expected.
I did spend some time thinking if that removal of dead code could cause any regressions, I couldn't really think of anything.
I think @michalkleiner 's early review question is still valid, so would be good to get an answer to that as well @sgiehl
Just want to de-risk the change as much as possible.
|
I've added a commit to reset the columns when an invalidation is released. Don't think that brings that much value, but might be more clean indeed. Besides that only one process should handle an invalidation, so it should not happen that one process overwrites the other. That already handled in the QueueConsumer. |
Description:
fixes #22970
Review