[PP1] Move memory reclamation out of migrate executable

All right, let's put it back. Maybe it would be better to start from a patch restoring the original code (including tests), then replace the old checkStatus() with the event?

I'm not fond of the resetCache() hack, I still think the entity system needs to use drupal_static() so it's possible to wipe the memory cache only.

+++ b/core/modules/migrate/src/EventSubscriber/MigratePostSaveSubscriber.php
@@ -0,0 +1,140 @@
+namespace Drupal\migrate_drupal\EventSubscriber;

Need to fix the namespace as well as move the file.

+++ b/core/modules/migrate/src/EventSubscriber/MigratePostSaveSubscriber.php
+  public function __construct($threshold = 0.85) {

Needs a doc block.

+++ b/core/modules/migrate/src/EventSubscriber/MigratePostSaveSubscriber.php
+            throw new MigrateException($this->t('Invalid PHP memory_limit !limit',
+              array('!limit' => $limit)));

Won't $this->t fail unless the subscriber imports StringTranslationTrait and accepts a StringTranslationInterface? onPostRowSave() also has calls to $this->t(), which are currently commented out.

+++ b/core/modules/migrate/src/EventSubscriber/MigratePostSaveSubscriber.php
+    // Entity storage can blow up with caches so clear them out.
+    $container = \Drupal::getContainer();
+    /** @var \Drupal\Core\Entity\EntityManagerInterface $manager */
+    $manager = $container->get('entity.manager');

Since the event subscriber is created by the container, there is no need to use \Drupal here. Can you inject the entity manager in the constructor?

Another thought occurs -- maybe $memoryThreshold should be a protected static variable which can altered by a static setter, in case outside code needs to change the memory threshold in mid-stream. Does that make any sense at all, or would it be a pointless edge case?

Beyond that, this looks OK to me. It needs tests, however.

Since there is already a need for big migrations to work smoothly, I think this should be escalated.

+++ b/core/modules/migrate/src/EventSubscriber/MigratePostSaveSubscriber.php
+  public function __construct($threshold = 0.85) {
+    $this->memoryThreshold = 0.85;
+  }

Should be $this->memoryThreshold = $threshold. Maybe also cast $threshold to a float.

+  public function getMemoryLimit() {
+
+    $limit = trim(ini_get('memory_limit'));

Nitpick -- thar be an extra line.

Should we remove the commented-out stuff and file a follow-up issue in its place?

Beyond this, it looks good to me. Still needs tests, though.

And let's file a follow-up issue about clearing the entity caches.

And let's file a follow-up issue about clearing the entity caches.

I submitted my patch at #2503999: Large volume entity migrations run out of memory.

Just wanted to bring up a comment I made in IRC - the event listener could be on MigrateExecutable rather than in a separate class. One advantage would be there wouldn't be any need to figure out how to pass the message interface or any other information from the executable to the listener.

+++ b/core/modules/migrate/src/EventSubscriber/MigratePostSaveSubscriber.php
@@ -0,0 +1,144 @@
+        throw new MigrateException();

Rather than throwing an exception, once #2545672: Handle various migration interruption scenarios lands we'll do $migration->setInterruptionResult(MigrationInterface::RESULT_INCOMPLETE) so the front-end tools can do whatever they need to do to resume the migration.

Starting with comments in #15 and a few other nitpicks. I renamed the function checkMemory() to something hopefully a bit more clearer. I will check on #17 as well.

#17: I am considering on how to put this into MigrateExecutable and I think I prefer this to be a separate class. It provides a separation of concerns and does not clutter up MigrateExecutable further.

As far as saving messages is concerned, the event already passed in the migration object. Can we not just call ->getIdMap()->saveMessage() on that? What do you think?

Can we not just call ->getIdMap()->saveMessage() on that?

These messages are not for saving in the message table (we don't really care what item happened to be importing when the memory threshold was hit), these are messages for display on the front-end.

This looks good although we should probably wait on #2546004: Add message service to migrate events rather than commit the commented out code?

Also still needs the tests restoring, setting to NW for that.

We have some barriers to getting this resolved ASAP:

1. This is a refactoring of the original code, so will require more review to be confident in RTBCing.
2. We don't have a test yet using the refactoring.
3. This refactoring is dependent on a new issue, #2546004: Add message service to migrate events.
4. With the memory checking being handled in an event rather than inline, we don't currently have a means to exit the executable import() loop when failing to reclaim enough memory - #2545672: Handle various migration interruption scenarios will provide that mechanism, but that's another external issue dependency.

I propose that we

1. Restore the memory reclamation exactly as it was - we should be able to turn that patch around quickly.
2. We get #2545672: Handle various migration interruption scenarios in next.
3. In a follow-up issue, we can make the reclamation event-driven, using #2545672: Handle various migration interruption scenarios to break the loop on reclamation failure.

This is a straight restoration of the original memory handling code, with one small improvement - replacing the constructor exception with a display() call.

@neclimdul: It seems that the patch in #27 also contains changes from #2546004-15: Add message service to migrate events. Was that intentional?

+++ b/core/modules/migrate/src/Event/MigrateImportEvent.php
@@ -25,10 +25,13 @@ class MigrateImportEvent extends Event {
+  public function __construct(MigrateMessageInterface $message, MigrationInterface $migration) {

Why not: $migration, $message same as MigrateExecutable

Ok I figured this issue would be a quick, ~10 minute fix to just put the code back in that was taken out, but this issue has now taken on a life of its own. In the meantime though we've lost capabilities that several people actively doing migrations have cited as important, and while we're in no danger of rolling a new beta right now, I don't think we want to accidentally put one out with this functionality missing.

Therefore, committed and pushed #26 (simple rollback) to 8.0.x to get us back to where we were prior to #2541580: Remove obsoleted functionality from core Migrate.

That will necessitate a re-roll of this one, but it can now proceed at whatever pace.

I also believe that makes this one much less severe than before, so removing Migrate critical tag and downgrading to normal. If that's wrong, please feel free to re-adjust.

The objective now is to decouple the memory reclamation from migrate core by using events and messages. Rather than renaming the title and issue summary to more accurately describe the new objective, is it better to create a followup? The title currently is "Re-add memory reclamation in migrate system" and we have already done that (in #26, which was committed). Should we mark this as fixed then?

We can do it here, don't think it matters to much.

Is postponed the right status here?

This is postponed on ... #2546004: Add message service to migrate events, right?

#2546004: Add message service to migrate events is in so this is no longer Postponed. Setting to NW since that what it was before being postponed. Not changing the title because I don't know the meaning of [PP1].

Just had a discussion in IRC with neclimdul about this and the child issue #2778433: Allow migration events to affect executable loop status.. I think the child issue is primarily motivated by the fact that right now, an exception thrown from a POST_ROW_SAVE handler would skip saveIdMapping(), which we don't want - we want to make sure the last imported item is properly recorded:

$destination_id_values = $destination->import($row, $id_map->lookupDestinationId($this->sourceIdValues));
$this->getEventDispatcher()->dispatch(MigrateEvents::POST_ROW_SAVE, new MigratePostRowSaveEvent($this->migration, $row, $destination_id_values));
// call saveIdMapping()

Looking at that code, I think it's wrong - POST_ROW_SAVE should be dispatched after saveIdMapping(). We want to maintain data integrity here - the import of an item, and the recording of its disposition, should not be separated by a potential exception from a POST_ROW_SAVE handler. Thus, my recommendation in IRC was:

1. Move the dispatch to after the idmap is saved.
2. Define a new exception, to be thrown from the POST_ROW_SAVE handler for memory reclamation.
3. Upon catching the exception, set $return to RESULT_INCOMPLETE and break the processing loop.

But, as I reacquaint myself with this section of the executable processing loop, we already have a mechanism for interrupting the loop - the POST_ROW_SAVE handler could simply do

$migration->interruptMigration(MigrationInterface::RESULT_INCOMPLETE);

and it should all work fine.

Thoughts?

Yeah, +1 for $migration->interruptMigration(MigrationInterface::RESULT_INCOMPLETE);, I don't really like the idea of adding another exception to control the flow.

The drupal_static_reset() call here won't do much any more in 8.x, since 97% of drupal_static() usage have been removed.

#1199866: Add an in-memory LRU cache might make the explicit memory reclaim unnecessary.

The last patch doesn't really reflect reality - effectively, the drupal_static_reset() call is being moved, not added, and it might as well stay to protect against any contrib users of drupal_static() building a large cache.

Most definitely looking forward to the entity cache issues being addressed, though!

There are too many commits and rollbacks in the issue. Opening a new issue and closing it as duplicate so we have a clean slate to start with. I hope that doesn't ruffle too many feathers.

There are too many commits and rollbacks in the issue. Opening a new issue and closing it as duplicate so we have a clean slate to start with.

Personally i think a clean state is worse, it's nice to have all the previous conversation around the issue, plus we have 15 people watching the issue.