Follow-up to #2485385: Move highwater field support to the source plugin, and do not expose its internals on MigrationInterface

Problem/Motivation

Per alexpott's comment:

Whilst reviewing this patch I pondered if we could set the highwater properly during rollback (see #125 and #127). After thinking about @mikeryan's response some more I think we should more the NULL setting from postRollback() to preRollback() this means that is the rollback fatals for any reason then the highwater is correctly indeterminate rather than wrong.

Good point - worst-case scenario setting it in preRollback(), you have to reimport everything if rollback fails. Worst-case scenario in postRollback() (as it is now), reimporting misses anything that was successfully rolledback - i.e., data loss.

Proposed resolution

Move the highwater code in SourcePluginBase from postRollback() to preRollback().

Remaining tasks

Implement the solution (Novice task).
Tests?

User interface changes

N/A

API changes

None

CommentFileSizeAuthor
#4 2800715-4.patch823 bytesshashikant_chauhan

Issue fork drupal-2800715

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mikeryan created an issue. See original summary.

juancasantito’s picture

juancasantito commented
Title: Reset highwater mark *before* rolling back » PP-1: Reset highwater mark *before* rolling back
Status: Active » Postponed

Waiting on #2485385: Move highwater field support to the source plugin, and do not expose its internals on MigrationInterface before working on this.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Title: PP-1: Reset highwater mark *before* rolling back » Reset highwater mark *before* rolling back
Status: Postponed » Active

not postponed any longer.

shashikant_chauhan’s picture

shashikant_chauhan commented
Status: Active » Needs review
StatusFileSize
new 2800715-4.patch823 bytes

adding patch.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Needs work
Issue tags: -Novice +Needs tests

Patch looks good, thanks!

Next step is a test (with a test-only patch to demonstrate what we're fixing). Probably not a novice task:

  1. Import a set of records, using a highwater mark.
  2. Rollback a set of records, aborting the rollback in the middle (probably simplest to do by calling $migration->interruptMigration() on a given record, say in a POST_ROW_DELETE handler).
  3. Rerun the import, and verify that all records are now imported (a count of records should suffice).

Without the fix, records that were successfully rolled back before the interruption will not be imported. With it, we should get the full count back.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Issue tags: +Bug Smash Initiative

This is a bugsmash target issue today. This is still valid and the starting point is still to add tests.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dcam made their first commit to this issue’s fork.

dcam opened merge request !11340

dcam’s picture

dcam commented
Status: Needs work » Needs review
Issue tags: -Needs tests

I didn't follow the recommended test plan. I created a unit test instead. Let me know if that isn't acceptable.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative 
1) Drupal\Tests\migrate\Unit\MigrateSourceTest::testPreRollback
Expectation failed for method name is "set" when invoked 1 time.
Method was expected to be called 1 time, actually called 0 times.
FAILURES!
Tests: 13, Assertions: 37, Failures: 1.
Exiting with EXIT_CODE=1

Shows the test coverage is there.

Migration is not my best but going off @quietone in #16 mentioning this is still valid as a migration maintainer.
Solution matches the MR

Believe this one is good, but if wrong I apologize.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 11.1.x-dev

Backported to 11.1.x as a bug fix.

Committed and pushed 420f7b2ff2d to 11.x and 180597712ad to 11.2.x and a94e3c35cb7 to 11.1.x. Thanks!

  • alexpott committed a94e3c35 on 11.1.x 
    Issue #2800715 by dcam, shashikant_chauhan, mikeryan, quietone, alexpott...

  • alexpott committed 18059771 on 11.2.x 
    Issue #2800715 by dcam, shashikant_chauhan, mikeryan, quietone, alexpott...

  • alexpott committed 420f7b2f on 11.x 
    Issue #2800715 by dcam, shashikant_chauhan, mikeryan, quietone, alexpott...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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