Problem/Motivation
The migration system is well suited for periodic, automated re-runs but if a migration run fails, the status never gets reset. Blindly forcing a reset is not ideal because there's not really a good way to detect whether a migration is really running (short of stracing the relevant process but I highly doubt we want to resort to that).
Also, the current system is race prone because the PRE_IMPORT
event fires between status check and status set. We have seen races even when the check and set were two instructions, one after the other, with an entire event between the two it's wide open.
Proposed resolution
We have a subsystem for this problem. The lock subsystem releases locks during shutdown and on top of that, they expire, too. Since backwards compatibility is a must, a new optional lock
section can be added to the migration definition with the following keys:
lock:
# This switches off the current behaviour of using status for locking
use_status: FALSE
# This switches on using the lock service.
use_lock: TRUE
# This is the lock timeout for the source iterator initalization.
initial_timeout: 60
# This is the lock timeout for every row.
timeout: 15
Remaining tasks
User interface changes
None at all.
API changes
New, optional section added to migration definitions.
Data model changes
None.
Release notes snippet
TODO.
Comment | File | Size | Author |
---|---|---|---|
#40 | 3066277-40.patch | 7.88 KB | andypost |
#40 | interdiff.txt | 1.63 KB | andypost |
#39 | 3066277-39.patch | 7.76 KB | heddn |
#39 | interdiff.txt | 4.64 KB | heddn |
#38 | 3066277-38.patch | 7.87 KB | andypost |
|
Comments
Comment #2
Ghost of Drupal PastComment #3
Ghost of Drupal PastComment #4
Ghost of Drupal PastComment #7
Ghost of Drupal PastComment #8
Ghost of Drupal PastDoh, I forgot to pass in the newly minted mocked lock.
Comment #10
vijaycs85AFAICS, the STATUS_IDLE exists to serve just this purpose. Does it mean, we could deprecate and remove it?
Not sure:
a) why we need to do this
b) part of this issue scope
c) if we have to:
1. Add comment to explain (esp why timeout/2).
2. probably add test coverage for cover this scenario?
d) Wonder if we need CR for this change.
Missing docblock.
Comment #11
Ghost of Drupal PastNot sure. UIs report the status -- drush surely does -- but I guess
lockMayBeAvailable
can be used to detect idleness.As for re-locking: while I could grab the lock with a very long timeout, I don't want to do that as it would lessen the advantages of using the lock subsystem. Rather, I grab the lock for a shorter period of time, renew it well before it expires , the half timeout is just as good as anything else, I could've chosen .8 or .9 instead of .5 but it doesn't matter much. Because we hold the lock only for half a minute, if PHP dies so abrutly the locks aren't released, the migration is ready to re-run in thirty seconds. I am not sure what change record is necessary here -- maybe there is , migration no longer need a status reset for sure but there's no harm in status resetting. I guess the CR is necessary to explain if you are really fast in re-run the status reset is no longer adequate -- either wait for the lock to time out or delete it from the database. I guess drush can add the latter to the status reset. Pity the generic lock backend doesn't offer a break functionality.
Comment #12
Ghost of Drupal PastI haven't deprecated STATUS_IDLE yet but if we want to, I can. This has a unit test, I factored out
testImportWithValidRow
and used it for two new tests. Is this the first usage of the splat operator in core? It's PHP 5.6 and we are 7.0 so I am not worried.Comment #13
Ghost of Drupal Pastmikeleutz raised a concern on Slack where the current system is actually useful and one wants to check things before manually resetting the status. So we have two different use cases here which calls for an option. Here.
Comment #14
heddnIn general I think using lock system has more pros than cons. I'm +1 in favor.
Nit: spelling
While strictly not part of BC, there are several multi thousand install contrib modules that extend the executable and this would hard break them.
Comment #15
Ghost of Drupal PastThanks heddn for the review and support.
The constructor is exempt: compare https://3v4l.org/sZLun to https://3v4l.org/lbP4V and the lock method fills in the lock service from
Drupal::lock()
if necessary.I will fix the typo or it could be fixed on commit if there is nothing else.
Comment #16
heddnIgnore #14.2. I see that isn't an issue.
A more thorough review of the rest of the patch:
Let's call this lock_timeout; more explicit.
This could (should?) use the getter for bypass status?
Nit: $bypassStatus variable name should be $bypass_status, I think.
Should this return
MigrationInterface::STATUS_IDLE
instead of magic number?Since we have a single implementation of the interface, could we just added these methods to the base interface since the base class will automatically implement? I think that is still safe for BC.
Comment #24
Ghost of Drupal PastRebooting with a new, configurable, backwards compatible, very simple approach: a new optional
lock
section can be added to the migration definition with the following keys:Comment #26
andypostthis method needs doc-block.
is it really required to be public as looking unused.
Comment #27
Ghost of Drupal PastComment #28
Ghost of Drupal PastComment #29
andypoststill not sure the getter is needed, you can use setAccessible() via reflection for the test
Comment #30
Ghost of Drupal PastComment #31
Ghost of Drupal PastI forgot to do rollback.
Comment #32
andypostIt looks ready just a nitpick
method arguments needs types as it's new
Comment #33
Ghost of Drupal PastComment #34
Ghost of Drupal PastComment #35
andypostLooks ready except debatable
I think as new code it should be typed as `protected LockBackendInterface $lock;`
Comment #36
heddnCan this be injected?
Why do we need to return an int|array? Can we return just a bool?
Comment #37
Ghost of Drupal PastI tried typing it above in a patch that's when phpcs failed, let me try again. But if we are typing our properties, do we have an issue to drop absolutely useless and superfluous doxygen? There's no point in adding "the lock backend" to `protected LockBackendInterface $lock`. That's just ridiculous.
Could we inject the lock backend? Of course it's possible. But MigrateExecutable is instantiated directly, there's no factory, there's no service. So unlike other classes where a new constructor argument was added and it caused little upheaval, here every test and every runner would need to be upgraded to avoid a deprecation error. And for what? By default it's not even used.
Why do we need to return an int|array? Because of this section
That's what makes the array part necessary. We need to convey somehow the lock is in use and how long the per row locking should be. If we so wanted we could implement a new exception thrown in initialize and caught in import/rollback. -- but that, again, seemed like a lot of work for very little benefit.
Comment #38
andypostI think lock should be injected via constructor as event dispatcher doing, it allows to simplify code a bit
Meantime
withConsecutive()
very probably will be deprecated so better to beware it see related #3306554: InvocationMocker::withConsecutive() is deprecated in PHPUnit 9.6 and removed from PHPUnit 10Comment #39
heddnThis doesn't overload the result from
initialize()
Comment #40
andypostFixed CS and add doc-block for method
Comment #42
Ghost of Drupal PastComment #44
Ghost of Drupal PastThanks for all input. I am deploying #37 to production. That comment explains why I am against adding an argument to the constructor and as for #39, I didn't feel like I needed a separate method to get the lock configuration. This is just a feeling of course. Please do not let this issue die despite I am not going to work on it further.