Follow-up to #2535458: Dispatch events at key points during migration

Problem/Motivation

When event dispatching is added to Migrate, a bunch of logically front-end responsibilities (such as time/memory limiting) now in the core service can be moved to contrib.

Proposed resolution

  1. Remove obsoleted code from the core Migrate module - support for statistics, time limits, memory limit handling, destination pre/post import/rollback methods that were never actually implemented. Statistics, time limits, memory handling will be the responsibility of front-ends running migrations (in practice, the migrate_tools module in migrate_plus).

Remaining tasks

  1. Split this patch out from the parent patch.
  2. Change record.

User interface changes

N/A.

API changes

Remove preImport(), preRollback(), postImport(), and postRollback() from MigrateDestinationInterface - these were never fully implemented. Implementers of this interface no longer need to define these methods.

Data model changes

N/A

CommentFileSizeAuthor
#7 interdiff.txt707 bytesmikeryan
#7 remove_obsoleted-2541580-7.patch29.46 KBmikeryan
#2 remove_obsoleted-2541580-2.patch29.7 KBmikeryan

Comments

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Postponed » Active

Events have landed! Now we can remove the obsolete crap...

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Active » Needs review
Issue tags: +Needs change record
Related issues: +#2443081: Make MigrateExecutable statistics publicly available, +#2536454: Use event listeners to implement a bunch of stuff
StatusFileSize
new remove_obsoleted-2541580-2.patch29.7 KB

9 files changed, 4 insertions(+), 790 deletions(-)

OK, here's what's being removed:

  • Various counters (number processed, number ignored, etc.). These were not used within core, and public methods to retrieve them were never implemented, so they weren't usable from contrib either. The migrate_tools module will be able to use events to track what's happening during migration.
  • pre/post Import/Rollback methods on DestinationInterface and DestinationBase - these were never called. This functionality is being provided by PRE_IMPORT/POST_IMPORT events (and equivalent rollback events, when we implement rollback).
  • Memory management during migration (e.g., reclaiming memory if getting too close to memory_limit) - this can now be done via events by the runners implemented in contrib (migrate_tools module in the migrate_plus project).
  • Time limit support (which didn't have a proper API, it was set on a public member variable) - this can be done via events in contrib.

There's one slight functional change - protected checkStatus() has been removed from MigrateExecutable, since all the running code it contained is removed. It did have a commented-out stub to allow migrations to be stopped from without (in particular, so if you have a migration running and want to cleanly abort it, you would run a drush migrate-stop command rather than Ctrl-C) - that bit is now implemented and in import() in place of the former call to checkStatus(). We could implement that in a separate patch if preferred.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

So much weight loss. Once there is a change record (and testbot passes it), I will RTBC.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented 
+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -353,10 +249,10 @@ public function import() {
+      // An POST_ROW_SAVE event listener may have set the status to STOPPING to

I'll call a nit on myself - s/An/A/. I'll wait to upload a fresh patch in case there's any other feedback to apply.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Issue tags: -Needs change record

Draft change record added.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

All looks great to me.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new remove_obsoleted-2541580-7.patch29.46 KB
new interdiff.txt707 bytes

Tabling the stoppage bit, we need to think about that in relation to other stoppage scenarios.

9 files changed, 792 deletions(-)

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Kewl.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Less code FTW!

My understanding from talking to @mikeryan and @phenaproxima (this is mentioned in the change notice as well) is that these interfaces, as tantalizing as they looked, were not actually functional. The Events system is a much more full-fledged replacement, and it makes no sense for core to ship with two versions of the same thing, especially when one is half-broken.

I do wonder if we're going to miss having things like memory consumption tracking and such in core, but I guess if we start missing those we could always add them back in 8.1.x+.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 1787b90 on 8.0.x 
    Issue #2541580 by mikeryan: Remove obsoleted functionality from core...
benjy’s picture

benjy commented 
+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -500,143 +351,6 @@ public function saveQueuedMessages() {
-  protected function attemptMemoryReclaim() {
-    // First, try resetting Drupal's static storage - this frequently releases
-    // plenty of memory to continue.
-    drupal_static_reset();
-    // @TODO: explore resetting the container.
-    return memory_get_usage();

It's a shame we had to remove all of this, things like memory re-claiming should be part of the core API and it seems we had plenty of tests for it as well.

I know we have events now so we can do stuff in contrib but there was nothing stopping us consuming those events in core rather than removing the code entirely. Would have been nice to get a review here, seemed to all happen pretty quickly.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Sorry, I was trying to clear the Migrate blockers. :( Jumped the gun a bit too soon.

Let's get an issue to add this back in.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

...for 8.0.x, not 8.1.x, in case that wasn't clear. Happy to commit it whenever it's ready.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

That's happening at #2545632: [PP1] Move memory reclamation out of migrate executable.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone’s picture

quietone commented

Published the change record.