Repository navigation
Support DMS migration during bundle deploy - #6989
Open
shreyas-goenka wants to merge 25 commits into
Open
shreyas-goenka wants to merge 25 commits into
shreyas-goenka wants to merge 25 commits into
Conversation
Collaborator
Integration test reportCommit: d97a698
Top 3 slowest tests (at least 2 minutes):
|
denik
reviewed
Oct 13, 2026
Contributor
This seems a bit narrow? In direct we have env var and config setting. Config setting is consulted first, then env var, then state-derived, then the default. Important also that state is sticky, so if you are on DMS and neither config nor env var are set, you still remain on DMS and not migrated back. Do we have that? The config setting/env var/stickiness is complex enough so that it warrants its own PR, what do you think? |
denik
reviewed
Oct 14, 2026
This was referenced Oct 14, 2026
shreyas-goenka
changed the base branch from
main
to
shreyas-goenka_data/shreyas-goenka/dms-serial-stack
October 14, 2026 12:22
shreyas-goenka
added this pull request to stack #7047
October 14, 2026 12:22
Base automatically changed from
shreyas-goenka_data/shreyas-goenka/dms-serial-stack
to
main
October 19, 2026 11:33
shreyas-goenka
force-pushed
the
shreyas-goenka_data/shreyas-goenka/dms-deploy-migration
branch
from
October 19, 2026 12:30
f90499d to
bc86051
Compare
denik
reviewed
Oct 19, 2026
| musterr trace DATABRICKS_BUNDLE_DEPLOYMENT_HISTORY=true $CLI bundle deploy | ||
| trace print_requests.py --dms //api/2.0/bundle --oneline | ||
| title "Normal state selection prefers local serial 2 over remote serial 1" | ||
| trace update_file.py .databricks/bundle/default/resources.json '"serial": 1' '"serial": 2' |
Contributor
There was a problem hiding this comment.
I don't understand why we have this serial update here? what its purpose, does migration below not work without it?
Contributor
Author
There was a problem hiding this comment.
No it absolutely does. This just simulates that if local state is ahead, the local state is used and thus the version created us at version 3.
I can see this test being a bit hard to read, let me simplify this just to the core assertions.
| // Upgrade from read (opened by process.go, or by Migrate) to write mode. After approval | ||
| // and the migration commit above, so a declined deploy never opens a WAL it must discard, | ||
| // and the migration's tf+1 push lands before this advances the WAL header to tf+2. | ||
| if err := b.DeploymentBundle.StateDB.UpgradeToWrite(); err != nil { |
Contributor
There was a problem hiding this comment.
Do we need this if we're on DMS?
Contributor
Author
There was a problem hiding this comment.
yeah because of the db.AssertOpenedForWrite() assertions we have at other places (like SaveState). I presume this is only meant to be an in memory guard in resources.json based deployments?
Contributor
There was a problem hiding this comment.
So we create empty WAL file here but don't write to it?
Perhaps AssertOpenedForWrite should handle both write mode and recording history mode.
Related, we should probably merge these into enum (reading, writingWAL, DMS)
// openedForWrite records write mode explicitly. It used to be implied by walFile, but a recorded
// deployment writes no WAL, so the two are no longer the same question.
openedForWrite bool
// recordsHistory is whether this deployment records history, decided by Open from the state's
// features. Cached because it outlives the features themselves: Finalize resets Data, and
// CompleteVersion runs after that.
recordsHistory bool
We don't want to open WAL when using DMS but SaveState should still work.
Contributor
Author
There was a problem hiding this comment.
Yeah makes sense. We'll like to have a readonly mode for DMS as well (for things like summary / validate). Can be a followup though.
Contributor
|
Commit: Regression test reportSelected 4 changed tests (limit=20, 0 not selected) 4 tests: regression=4 unreleased=0 coverage=0 failing=0
TestAccept/bundle/dms/existing-state/DMS=: branch ✅ | main (a1cf984) ❌ | latest (v1.21.0) ❌main (a1cf984): latest (v1.21.0): TestAccept/bundle/dms/migration-before-deploy/DMS=true: branch ✅ | main (a1cf984) ❌ | latest (v1.21.0) ❌main (a1cf984): latest (v1.21.0): TestAccept/bundle/dms/repeated-migration/DMS=true: branch ✅ | main (a1cf984) ❌ | latest (v1.21.0) ❌main (a1cf984): latest (v1.21.0): TestAccept/bundle/migrate/auto/dms-migration/DMS=: branch ✅ | main (a1cf984) ❌ | latest (v1.21.0) ❌main (a1cf984): latest (v1.21.0): |
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Oct 20, 2026
Follow-up to #7105. The comments on #6989 and #7109 were 34-44 KB, close to the 60 KB cap, mostly because a new test fails the same way on main and on the latest release and the same diff was printed twice. - A failing run whose output equals an earlier column's (ignoring the random temp dir) now says "same output as <column>". - When the comment is over the cap, whole failure details are dropped (with a count) instead of cutting one in the middle and leaving its markup open. This pull request and its description were written by Isaac. --------- Co-authored-by: Isaac <no-reply@databricks.com>
denik
reviewed
Oct 21, 2026
| trace print_requests.py --dms //api/2.0/bundle --oneline | ||
| trace jq '{state_version, features}' .databricks/bundle/default/resources.json | ||
| title "1. Create two jobs without history (direct state serial 1)\nConfig disables history even though the environment enables it." | ||
| trace DATABRICKS_BUNDLE_DEPLOYMENT_HISTORY=true $CLI bundle deploy 2>&1 | contains.py '!Current Deployment Version:' '!Enabling deployment history' |
Contributor
There was a problem hiding this comment.
Why enable DATABRICKS_BUNDLE_DEPLOYMENT_HISTORY=true here? Do you test env + config combination there? Was it not tested in #7045 ? If there is missing coverage for that feature, perhaps it can be done as separate PR?
| @@ -1,38 +1,40 @@ | |||
| envsubst < databricks.yml.tmpl > databricks.yml | |||
|
|
|||
| title "Deploy without recording: ordinary direct-engine state tracking one job, unknown to DMS" | |||
Contributor
There was a problem hiding this comment.
general observation, this test is rewritten so much it is practically a different test. Maybe worth adding this new test separately and record output changes for the original test (with minimal script changes).
| other_catalog="dms_other_${UNIQUE_NAME}" | ||
|
|
||
| title "Setup (LOG.setup): create a job and schema without history\nNext deploy enables history and recreates the schema in another catalog, requiring approval." | ||
| { |
Contributor
There was a problem hiding this comment.
Why do we need to group it? cannot we record the command + output with trace? This makes output easier to read. As far as I can see the commands below have no or very little output.
| title "Setup (LOG.setup): create a job and schema without history\nNext deploy enables history and recreates the schema in another catalog, requiring approval." | ||
| { | ||
| $CLI bundle deploy -qq | ||
| DATABRICKS_BUNDLE_DEPLOYMENT_HISTORY=false replace_ids.py |
Contributor
There was a problem hiding this comment.
Is it significant to have env var here that matches config?
| // Upgrade from read (opened by process.go, or by Migrate) to write mode. After approval | ||
| // and the migration commit above, so a declined deploy never opens a WAL it must discard, | ||
| // and the migration's tf+1 push lands before this advances the WAL header to tf+2. | ||
| if err := b.DeploymentBundle.StateDB.UpgradeToWrite(); err != nil { |
Contributor
There was a problem hiding this comment.
So we create empty WAL file here but don't write to it?
Perhaps AssertOpenedForWrite should handle both write mode and recording history mode.
Related, we should probably merge these into enum (reading, writingWAL, DMS)
// openedForWrite records write mode explicitly. It used to be implied by walFile, but a recorded
// deployment writes no WAL, so the two are no longer the same question.
openedForWrite bool
// recordsHistory is whether this deployment records history, decided by Open from the state's
// features. Cached because it outlives the features themselves: Finalize resets Data, and
// CompleteVersion runs after that.
recordsHistory bool
We don't want to open WAL when using DMS but SaveState should still work.
This branch has not been deployed
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.
Changes
Enabling
experimental.deployment_historyfor an existing bundle target will automatically migrate the bundle.resources.jsontoresources.json.backupbefore migration.INITIAL_REGISTERin a dedicated version, then apply deployment changes in a subsequent version.max(state serial, latest DMS version) + 1. Re-register when the selected state has no deployment-history marker, including repeated migration.Migration uses the already-selected, open state and acquires its own deployment lock only when migration is needed. Once committed, it remains committed even if the subsequent deployment fails planning or is declined. Normal deployment has no pending-migration object or migration-specific plan patching. Read-only plan commands do not commit migration.
Tests