Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add cli command to migrate job runs from one repo to another #9376

Merged
merged 7 commits into from
Oct 7, 2022

Conversation

prha
Copy link
Member

@prha prha commented Aug 15, 2022

Summary & Motivation

Context: #9317

Enables refactoring of repo code, while preserving history

How I Tested These Changes

BK

@vercel
Copy link

vercel bot commented Aug 15, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Ignored Deployments
Name Status Preview Comments Updated
dagit-storybook ⬜️ Ignored (Inspect) Oct 7, 2022 at 9:42PM (UTC)
dagster ⬜️ Ignored (Inspect) Oct 7, 2022 at 9:42PM (UTC)
dagster-oss-cloud-consolidated ⬜️ Ignored (Inspect) Oct 7, 2022 at 9:42PM (UTC)

@prha prha requested review from gibsondan and sryza August 15, 2022 19:39
@sryza sryza requested a review from alangenfeld August 16, 2022 23:00
Copy link
Contributor

@sryza sryza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable, though I'm not super well versed in the part of the code, so I think it could be worth it for @alangenfeld to take a look as well.

python_modules/dagster/dagster/_cli/run.py Outdated Show resolved Hide resolved
Copy link
Member

@alangenfeld alangenfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the origin in the serialized DagsterRun will be out of sync, we use that as source of truth for idempotence checks.

So if you try to move a job that is hooked up to a sensor with run_key idempotence, the old run key runs will get discarded when we see a non matching origin id


if should_migrate:
for record in tqdm(records):
with instance.run_storage.connect() as conn:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep connection through loop - no need to reconnect each iteration

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

little spicy depending on run_storage.connect here like this but i suppose the test helps protect (at least the one impl that the test uses)

@prha prha force-pushed the prha/run_repo_tag_migration_cli branch from dd35a38 to 5dfafe3 Compare August 24, 2022 17:53
@prha prha requested review from sryza and alangenfeld August 25, 2022 00:24
@prha prha force-pushed the prha/run_repo_tag_migration_cli branch from 0a8ad73 to 1a38e26 Compare August 25, 2022 17:59
Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

latest version here seems reasonable to me, any lingering concerns alex?

Comment on lines 128 to 129
if not isinstance(instance.run_storage, SqlRunStorage):
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raise?

Copy link
Member

@alangenfeld alangenfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope

@prha prha force-pushed the prha/run_repo_tag_migration_cli branch from 1a38e26 to d9aa097 Compare October 7, 2022 20:20
prha added 2 commits October 7, 2022 13:32
[INTERNAL_BRANCH=prha/run_migration]
@prha prha force-pushed the prha/run_repo_tag_migration_cli branch from 83e1c98 to a889783 Compare October 7, 2022 21:42
@prha prha merged commit bb3bf95 into master Oct 7, 2022
@prha prha deleted the prha/run_repo_tag_migration_cli branch October 7, 2022 23:13
clairelin135 added a commit that referenced this pull request Oct 7, 2022
test #2

change to set cursor to latest materializations

add cli command to migrate job runs from one repo to another (#9376)

* add cli command to migrate job runs from one repo to another

* extract migration into sql run storage

* fix tests

* remove prompt_required arg

[INTERNAL_BRANCH=prha/run_migration]

* rebase; raise;

* fix rebase

* fix bad rebase

[INTERNAL_BRANCH=prha/run_migration]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants