You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dstandish (via GitHub)" <gi...@apache.org> on 2025/05/19 17:03:53 UTC
[PR] Skip empty dag run conf rows and set statement timeout [airflow]
dstandish opened a new pull request, #50788:
URL: https://github.com/apache/airflow/pull/50788
Don't bother depickling `{}` object in dag run conf
Also, set the statement timeout a little higher for xcom migration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip empty dag run conf rows and set statement timeout [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #50788:
URL: https://github.com/apache/airflow/pull/50788#discussion_r2110293821
##########
airflow-core/docs/img/airflow_erd.sha256:
##########
Review Comment:
i believe it is because the migration is changing and it must be the case that migration files are included in the hash
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip empty dag run conf rows and set statement timeout [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish merged PR #50788:
URL: https://github.com/apache/airflow/pull/50788
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip empty dag run conf rows and set statement timeout [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #50788:
URL: https://github.com/apache/airflow/pull/50788#discussion_r2098391278
##########
airflow-core/docs/img/airflow_erd.sha256:
##########
@@ -1 +1 @@
-4cda55eb221ee0340749c8dd41af7603220d7b300e2e808d733239a86e0c2837
\ No newline at end of file
+80d87cc89c29fd73466a8625f82dfcbff347e9e72fee498bcb61a0acb90bf3a1
Review Comment:
does it perhaps regenerate the hash whenever you change the migration files? if the static check didn't fail, it must be right
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip empty dag run conf rows and set statement timeout [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on PR #50788:
URL: https://github.com/apache/airflow/pull/50788#issuecomment-2897972353
> Should we create a new migration file instead of updating already released migrations? For new release, it would appear as if there's no migration on the version, and upgrading from a previous one like 3.0.1 won't run the migration.
I don't think so -- we're just optimizing the existing migrations. We've done this many times before.
If a user is on 3.0.1, they've already passed the problematic migrations. So it remains true that there is no migration between 3.0.1 and 3.0.2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip empty dag run conf rows and set statement timeout [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #50788:
URL: https://github.com/apache/airflow/pull/50788#discussion_r2109725600
##########
airflow-core/docs/img/airflow_erd.sha256:
##########
Review Comment:
Why is this checksum changing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip empty dag run conf rows and set statement timeout [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #50788:
URL: https://github.com/apache/airflow/pull/50788#discussion_r2096414778
##########
airflow-core/src/airflow/migrations/versions/0049_3_0_0_remove_pickled_data_from_xcom_table.py:
##########
@@ -77,9 +77,24 @@ def upgrade():
condition = condition_templates.get(dialect)
if not condition:
raise RuntimeError(f"Unsupported dialect: {dialect}")
-
# Key is a reserved keyword in MySQL, so we need to quote it
quoted_key = conn.dialect.identifier_preparer.quote("key")
+ if dialect == "postgresql":
+ curr_timeout = (
+ int(
+ list( # noqa: RUF015
+ conn.execute(
+ text("""select setting
+ from pg_settings
+ where name = 'statement_timeout'""")
+ )
+ )[0][0]
+ )
+ / 1000
+ )
Review Comment:
This is a little unwieldy, can it be simplified?
```suggestion
curr_timeout = int(
conn.execute(
text("""select setting from pg_settings where name = 'statement_timeout'""")
).scalar_one()
) / 1000
```
Something like this maybe (untested)?
##########
airflow-core/src/airflow/migrations/versions/0055_3_0_0_remove_pickled_data_from_dagrun_table.py:
##########
@@ -61,12 +74,20 @@ def upgrade():
""")
)
else:
- BATCH_SIZE = 100
+ BATCH_SIZE = 1000
offset = 0
while True:
+ err_count = 0
+ batch_num = offset + 1
+ print(f"converting dag run conf. batch={batch_num}")
rows = conn.execute(
text(
- f"SELECT id,conf FROM dag_run WHERE conf IS not NULL order by id LIMIT {BATCH_SIZE} OFFSET {offset}"
+ "SELECT id, conf "
+ "FROM dag_run "
+ "WHERE conf IS not NULL "
+ f"AND conf != {empty_val}"
Review Comment:
We can just ignore them vs updating them to `{}`?
##########
airflow-core/docs/img/airflow_erd.sha256:
##########
@@ -1 +1 @@
-4cda55eb221ee0340749c8dd41af7603220d7b300e2e808d733239a86e0c2837
\ No newline at end of file
+80d87cc89c29fd73466a8625f82dfcbff347e9e72fee498bcb61a0acb90bf3a1
Review Comment:
hmm, bad conflict resolution? Don't think the other changes should change this.
##########
airflow-core/src/airflow/migrations/versions/0055_3_0_0_remove_pickled_data_from_dagrun_table.py:
##########
@@ -85,14 +106,17 @@ def upgrade():
"""),
{"json_data": json_data, "id": row_id},
)
- except Exception as e:
- print(f"Error converting dagrun conf to json for dagrun ID {row_id}: {e}")
+ except Exception:
+ err_count += 1
continue
+ if err_count:
+ print(f"could not convert dag run conf for {err_count} records. batch={batch_num}")
offset += BATCH_SIZE
op.drop_column("dag_run", "conf")
op.alter_column("dag_run", "conf_json", existing_type=conf_type, new_column_name="conf")
+ print(f"end: {pendulum.now()}")
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip empty dag run conf rows and set statement timeout [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #50788:
URL: https://github.com/apache/airflow/pull/50788#discussion_r2098389258
##########
airflow-core/src/airflow/migrations/versions/0049_3_0_0_remove_pickled_data_from_xcom_table.py:
##########
@@ -77,9 +77,24 @@ def upgrade():
condition = condition_templates.get(dialect)
if not condition:
raise RuntimeError(f"Unsupported dialect: {dialect}")
-
# Key is a reserved keyword in MySQL, so we need to quote it
quoted_key = conn.dialect.identifier_preparer.quote("key")
+ if dialect == "postgresql":
+ curr_timeout = (
+ int(
+ list( # noqa: RUF015
+ conn.execute(
+ text("""select setting
+ from pg_settings
+ where name = 'statement_timeout'""")
+ )
+ )[0][0]
+ )
+ / 1000
+ )
Review Comment:
i'll try it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip empty dag run conf rows and set statement timeout [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50788:
URL: https://github.com/apache/airflow/pull/50788#issuecomment-2914256660
### Backport failed to create: v3-0-test. View the failure log <a href='https://github.com/apache/airflow/actions/runs/15286552238'> Run details </a>
<table>
<tr>
<th>Status</th>
<th>Branch</th>
<th>Result</th>
</tr>
<tr>
<td>❌</td>
<td>v3-0-test</td>
<td><a href="https://github.com/apache/airflow/commit/f75b60740e59d47a223b350f506d9100e76c584d"><img src='https://img.shields.io/badge/Commit-f75b607-red' alt='Commit Link'></a></td>
</tr>
</table>
You can attempt to backport this manually by running:
```bash
cherry_picker f75b607 v3-0-test
```
This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.
After you have resolved the conflicts, you can continue the backport process by running:
```bash
cherry_picker --continue
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip empty dag run conf rows and set statement timeout [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #50788:
URL: https://github.com/apache/airflow/pull/50788#discussion_r2098392968
##########
airflow-core/src/airflow/migrations/versions/0055_3_0_0_remove_pickled_data_from_dagrun_table.py:
##########
@@ -61,12 +74,20 @@ def upgrade():
""")
)
else:
- BATCH_SIZE = 100
+ BATCH_SIZE = 1000
offset = 0
while True:
+ err_count = 0
+ batch_num = offset + 1
+ print(f"converting dag run conf. batch={batch_num}")
rows = conn.execute(
text(
- f"SELECT id,conf FROM dag_run WHERE conf IS not NULL order by id LIMIT {BATCH_SIZE} OFFSET {offset}"
+ "SELECT id, conf "
+ "FROM dag_run "
+ "WHERE conf IS not NULL "
+ f"AND conf != {empty_val}"
Review Comment:
yes i inferred this because, that's what we do every time we fail to convert anyway -- when the pickled object is not json serializable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org