-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Update dag reserialize command #43949
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
Conversation
|
Approved. We need it now. Though I think it's quite a big limitation. Possibly (@jedcunningham ) - with DAG bundles we could actually in the future to reserialize also history ? It sounds feasible, we would just have to go through version history and checkout the bundle in each version and reserialize it then. It also might (or might not) work - depends for example on the third-party package versions installed But yeah, that's likely Airflow 3.5+ or smth :) |
|
Maybe also we should attempt to keep old serialized DAGS and attempt to load them (Assuming that our serialization is forward-compatible) . That could also be done on a "best effort" case. |
Yeah, old serialized dags won't be deleted except users want to do so. We would keep a serialization version that when deserializing, we could check the version it was serialized with and use that to deserialize it. It should be forward-compatible |
This can already be pretty slow. Doing it for every historical version isn't an option imo.
I think something like this is the right way to handle this. I think Kaxil is going to look into it as part of AIP-72 stuff: #43648 |
3061cdf to
67db5b2
Compare
|
Given users often run this as part of upgrades (or db upgrade does it itself) we'll need to stop deleting things very soon |
67db5b2 to
33132cb
Compare
|
I'm thinking of renaming this command as |
33132cb to
779e528
Compare
|
Will appreciate another review @jedcunningham @pierrejeambrun |
pierrejeambrun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not yet super familiar with the Versioning AIP but this makes sense to me.
Thanks.
I think re-version is probably not a good name. Shall we chat about it? |
dstandish
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think we may want to revisit some of the language changes. blocking merge @ephraimbuddy has chance to respond
779e528 to
2043c5e
Compare
abb2b72 to
f80eb47
Compare
dstandish
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple suggestions
d55dd69 to
a02968a
Compare
Now that we have versioning, users must be sure they want to use the dags reserialize command, as it deletes dag history. I updated the command to ensure users know it will delete DAG history and answer yes for it to continue. The DagVersion is deleted and since it has foreignkey to SerializedDagModel and DagCode, those will also get deleted. Also updated the _reserialize function at DB upgrade so that it doesn't delete the serializedDag since that won't be necessary Updated the test to use session fixture instead of create_session
a02968a to
34842af
Compare
* Update dag reserialize command Now that we have versioning, users must be sure they want to use the dags reserialize command, as it deletes dag history. I updated the command to ensure users know it will delete DAG history and answer yes for it to continue. The DagVersion is deleted and since it has foreignkey to SerializedDagModel and DagCode, those will also get deleted. Also updated the _reserialize function at DB upgrade so that it doesn't delete the serializedDag since that won't be necessary Updated the test to use session fixture instead of create_session * add --clear-history to dag reserialize command * Add news fragment item * Remove clear-history * fix test * fixup! fix test * Update newsfragments/43949.significant.rst
Now that we have versioning, users must specify that they want to delete history before we do it in
airflow dags reserializecommand.serialize dags are no longer deleted as part of this command. Users should use airflow db-clean command
Also updated the _reserialize function at DB upgrade so that it doesn't delete the serializedDag since that won't be necessary.