[Migration Engine Part 3] Implement the RavenDB to SQL migration machinery - #5911
Draft
warwickschroeder wants to merge 7 commits into
Draft
warwickschroeder wants to merge 7 commits into
warwickschroeder wants to merge 7 commits into
Conversation
johnsimons
reviewed
Sep 25, 2026
Adds the RavenDB source and the EF Core target for the KnownEndpoints and EndpointSettings categories, the startup checks that refuse an unsupported or unready migration, and the stall watchdog that stops a copy committing nothing for 30 minutes. Documents the migration contracts.
Adds unit tests for the startup checks, the stall watchdog and the refusals, target and reader tests for both persisters, a SQL Server collation test for the endpoint settings key, and acceptance tests for a copy that is killed, restarted and opened on. Approves the two new migration settings.
…mpatibility across migration targets
…ings and behavior
warwickschroeder
force-pushed
the
warwick/migration-engine-3
branch
from
September 30, 2026 08:23
3fc7d09 to
58d4114
Compare
rbev
approved these changes
Oct 2, 2026
Contributor
There was a problem hiding this comment.
Is the plan to leave this here or promote the "how to migrate" to the docs site?
| **Whole categories are never copied.** Which ones, and why nothing needs them, is the [not migrated](#not-migrated) list above. Anything in an optional category you did not select is also never copied, and nothing later goes back for it. Neither is an event log item raised before the start of the 7-day window, which falls outside the [required](#required) event log window rather than being skipped. | ||
|
|
||
| **Rows skipped one at a time, and counted.** Each of these shows up in the skipped count for its category, broken out by reason, so you can see how much went and why: | ||
| **Rows skipped one at a time, and counted.** Each of these shows up in the skipped count for its category, broken out by reason, so you can see how much went and why. Only three skip reasons exist in code today: a body that could not be read, a row missing a value SQL requires, and settings for an unknown endpoint. Each of the other rules below needs a new reason value before it can be written at all, because the checkpoint refuses a batch whose skips do not add up. |
Contributor
There was a problem hiding this comment.
I don't love this way of phrasing this. Should rulesets like this be tabularised somewhere?
| - **Processing attempt history collapses to the newest attempt.** The SQL model has no attempts table. This affects every failed message that failed more than once, whether it is unresolved, archived or resolved. A message that failed five times arrives showing one attempt, and the other four are gone. | ||
| - **Subscriptions that differ only in message-type version merge onto one row**, because the target key carries the type name without the version. | ||
| - **Endpoint settings for two endpoint names that differ only in case merge onto one row on SQL Server**, because SQL Server's default collation compares names without case, so one of the two settings is kept. PostgreSQL keeps both, and so does a SQL Server database created with a case-sensitive collation. The dry run counts this one too, by asking SQL Server how the name column compares, though for unusual characters its count can differ from what the copy does. | ||
| - **Endpoint settings for two endpoint names that differ only in case merge onto one row on SQL Server**, because the collation of the name column decides the comparison and the default collation compares names without case, so one of the two settings is kept. It is the column's own collation that decides, not the database default, so a case-sensitive database whose name column was given a case-insensitive collation still merges. PostgreSQL keeps both. The dry run counts this one too, by asking SQL Server how the name column compares, though for unusual characters its count can differ from what the copy does. |
Contributor
There was a problem hiding this comment.
This is a functional change that could impact licensing, is it actually ok?
| - **A small category is not protected by the floor.** A separate rule halts any category that lost more than half its rows, whatever the floor says, because a category smaller than the floor would otherwise never reach it however much of it was lost. | ||
| - **A category also halts if it reaches the end of the source short.** If copied, skipped and already-present rows together come to less than the source count taken at the start, the category halts even though no threshold was crossed. **Planned:** before halting, the copier counts the source again, and halts only if the missing rows still exist in RavenDB, because rows RavenDB expired during the copy are an absence rather than a loss. | ||
| - Rows left behind because SQL would remove them anyway are counted and reported, but never halt a category. Today this exemption covers exactly one reason, settings for an unknown endpoint, and it withdraws itself: if the known endpoints copy skipped anything, an unknown endpoint can be this migration's own doing, so those skips start counting toward a halt again. | ||
| - The percentage is measured against what the run has processed so far rather than against the category's total, so a run that starts badly looks worse than it is. The floor is what keeps that harmless in a large category, since fewer than 101 skipped rows never consults the percentage at all. More than that, bunched at the start, does halt a category whose overall rate would have been fine, and the cost is one restart: the skipped rows commit with the cursor, so the next run resumes past them with its counters back at zero. |
Contributor
There was a problem hiding this comment.
Is this behaviour what we want?
If this is being run in a containerised environment they very often have auto-restart enabled.
Comment on lines
+12
to
+13
| // Mandatory, not stylistic: this assembly is Parallelizable(ParallelScope.All) and these fixtures set | ||
| // process-global environment variables. One fixture added without it makes the whole suite intermittent. |
Contributor
There was a problem hiding this comment.
Unnecessary comment, the fact that it's here implies that it's needed, the comment does not actually provide any justification beyond that.
Suggested change
| // Mandatory, not stylistic: this assembly is Parallelizable(ParallelScope.All) and these fixtures set | |
| // process-global environment variables. One fixture added without it makes the whole suite intermittent. |
Although if this really is being applied across the whole test library can you just change the assembly default or put the attribute on the base class?
|
|
||
| foreach (var row in batch.Rows) | ||
| { | ||
| var endpoint = (KnownEndpoint)row.Document; |
Contributor
There was a problem hiding this comment.
If these made use of a generic typed base class you could have a test that enforced the reader/writer pair are using the same payload types to make this cast safer.
|
|
||
| public Task<long> Count(MigrationCategory category, CancellationToken cancellationToken = default) => | ||
| throw new NotSupportedException($"The RavenDB migration source cannot count category {category.Id} yet"); | ||
| // Counted by streaming the same documents Read walks, not from RavenDB's collection statistics: a total that |
Contributor
There was a problem hiding this comment.
Is this being overly pessimistic?
| $"The RavenDB database '{databaseName}' carries no ServiceControl data version stamp. Start this instance once on RavenDB with version {RavenDataVersion.Current} before setting {MigrationSettings.EnabledKey}, so the source is brought up to date and stamped."); | ||
| } | ||
|
|
||
| // RavenDB deserializes with Newtonsoft, which ignores the required modifier, so a document saved without |
Contributor
There was a problem hiding this comment.
This might be a bit of a future footgun since newer Newtonsoft versions do honor the required attributes. Is there a test that provides for that regression?
| } | ||
|
|
||
| [Test] | ||
| public async Task A_category_smaller_than_the_floor_that_loses_every_row_halts_rather_than_completing() |
Contributor
There was a problem hiding this comment.
Should the floor be a percentage of the total instead when the total is low? Maybe 10%?
| /// Refuses any target but SQL Server or PostgreSQL, since the source is always RavenDB. The seams underneath are | ||
| /// general enough to copy between any two persisters, and this is the check that says which pair is actually supported. | ||
| /// </summary> | ||
| class MigrationPairIsSupportedCheck(Settings settings) : IMigrationStartupCheck |
Contributor
There was a problem hiding this comment.
Could this be driven of metadata in the manifests instead of being static to our current use case?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this adds
The machinery that copies an instance's data from RavenDB into SQL Server or PostgreSQL during startup, before the host opens. Only known endpoints and endpoint settings can be copied, so a real instance with
Migration/Enabledset still refuses to start; message bodies are not read, andMigration/AllowIncompleteExitdoes nothing yet.Tests
ServiceControl.UnitTests/Migration: the engine, each startup check, the stall watchdog and the refusal messages, against fakes and a fake clock.ServiceControl.Persistence.Tests/EFCore/Migration: the target and its two writers on SQL Server and PostgreSQL, and a check that fails when an entity has no migration decision.ServiceControl.Persistence.Tests.RavenDB/DataMigration: the readers, the source and its data version against an embedded server.ServiceControl.Persistence.Tests.SqlServer: endpoint settings keys that differ only in case, and settings wiring.ServiceControl.Migration.AcceptanceTests(new): a killed copy, a restart, each refusal and the host opening on the target, on both providers.ServiceControl.Migration.Tests: every category with a reader has a writer, and the reverse.