You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "o-nikolas (via GitHub)" <gi...@apache.org> on 2024/03/11 22:48:32 UTC
[PR] Add executor field to the DB and parameter to the operators [airflow]
o-nikolas opened a new pull request, #38054:
URL: https://github.com/apache/airflow/pull/38054
This field will be used by AIP-61 code to store and retrieve which executor a task is meant to run on.
This PR adds the new field and parameter but does not make any use of it just yet. No documentation is provided, intentionally, since the AIP-61 feature is not yet complete. I am slowly implementing pieces instead of one giant PR. A warning is shown if a user attempts to provide a value for this parameter to any operators.
I've done manual UAT with these changes (hello world dag, triggere dag, mapped task, task flow, etc) and all seems to be functioning.
I have some open question in the code which I've annotated with comments in the PR as well.
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1523842432
##########
airflow/models/baseoperator.py:
##########
@@ -635,6 +639,7 @@ class derived from this one results in the creation of a task object,
runs across execution_dates.
:param max_active_tis_per_dagrun: When set, a task will be able to limit the concurrent
task instances per DAG run.
+ :param executor: which executor to target when running this task. NOT YET SUPPORTED
Review Comment:
Nittiest of nitpicks. Some are capitalized, some aren't, feel free to ignore
```suggestion
:param executor: Which executor to target when running this task. NOT YET SUPPORTED
```
##########
tests/www/views/test_views_tasks.py:
##########
@@ -1089,6 +1089,7 @@ def test_task_instances(admin_client):
"duration": None,
"end_date": None,
"execution_date": DEFAULT_DATE.isoformat(),
Review Comment:
Not expecting you to change it but wow is that test incredibly repetitive. Maybe after this is merged I'll do a quick PR to move all the fixed values into a dict and just unpack then in each of these. Would make it much easier to read and the only values that are called out explicitly would be the ones that are changing and worth paying specific attention to.
##########
airflow/models/baseoperator.py:
##########
@@ -719,6 +724,8 @@ class derived from this one results in the creation of a task object,
"on_skipped_callback",
"do_xcom_push",
"multiple_outputs",
+ # TODO: should executor go here? I think so
+ "executor",
Review Comment:
Yeah, that's awkward. I think it goes in there, seems to make sense to me.
##########
airflow/models/taskinstance.py:
##########
@@ -1252,6 +1254,8 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
queued_by_job_id = Column(Integer)
pid = Column(Integer)
+ # TODO: Should this be nullable?
+ executor = Column(String(1000))
Review Comment:
I don't think nullable makes sense here, as uranusjr said.. can't think of a case where "no executor" would be valid?
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2013522650
@uranusjr @potiuk I've addressed all the comments, done a fresh rebase and tests are passing. Can I get another a approval to merge this one?
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1525311303
##########
airflow/models/abstractoperator.py:
##########
@@ -58,6 +58,8 @@
DEFAULT_OWNER: str = conf.get_mandatory_value("operators", "default_owner")
DEFAULT_POOL_SLOTS: int = 1
DEFAULT_PRIORITY_WEIGHT: int = 1
+# TODO: Is None the right thing here? We want NULL essentially
Review Comment:
Good question, it's covered in more detail in the AIP (I wish I could hyper link to one specific section or comment...) but essentially NULL, means that no specific executor was requested for the task. In those cases we fall back to the historical/default behaviour of Airflow, which is to use whatever the currently configured environment level executor is, which may change at any time (if the user decides to switch 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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1521964319
##########
airflow/models/taskinstance.py:
##########
@@ -1252,6 +1254,8 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
queued_by_job_id = Column(Integer)
pid = Column(Integer)
+ # TODO: Should this be nullable?
+ executor = Column(String(1000))
Review Comment:
What would the NULL case mean? Compared to, say, an empty string, or the default executor value?
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2016878718
Yep. We cannot merge it without resolving conflict :(
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi merged PR #38054:
URL: https://github.com/apache/airflow/pull/38054
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1524043624
##########
airflow/migrations/versions/0137_2_9_0_add_new_executor_field_to_db.py:
##########
@@ -0,0 +1,48 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""add new executor field to db
+
+Revision ID: 677fdbb7fc54
+Revises: ab34f260b71c
+Create Date: 2024-03-11 15:26:59.186579
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+
+
+# revision identifiers, used by Alembic.
+revision = '677fdbb7fc54'
+down_revision = 'ab34f260b71c'
+branch_labels = None
+depends_on = None
+airflow_version = '2.9.0'
+
+
+def upgrade():
+ """Apply add executor field to task instance"""
+ # NOTE: I cannot find a straightforward sdk doc for SA types. I don't know
+ # if I need default=None. No idea how to tell what are the defaults without docs
Review Comment:
But I guess we want to set default in this case (see below) - so likely we need to set default to defailt executor config.
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1524042120
##########
airflow/migrations/versions/0137_2_9_0_add_new_executor_field_to_db.py:
##########
@@ -0,0 +1,48 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""add new executor field to db
+
+Revision ID: 677fdbb7fc54
+Revises: ab34f260b71c
+Create Date: 2024-03-11 15:26:59.186579
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+
+
+# revision identifiers, used by Alembic.
+revision = '677fdbb7fc54'
+down_revision = 'ab34f260b71c'
+branch_labels = None
+depends_on = None
+airflow_version = '2.9.0'
+
+
+def upgrade():
+ """Apply add executor field to task instance"""
+ # NOTE: I cannot find a straightforward sdk doc for SA types. I don't know
+ # if I need default=None. No idea how to tell what are the defaults without docs
Review Comment:
Looking at other migrations - default is the Python default value assigned when the object is created not a server side default - which means that existing rows will get the default. And there are already a number of migrations that do not have defaults, so if you do not want to set a value, omitting default should work. But of course testing it is a good idea.
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1530779958
##########
airflow/models/taskinstance.py:
##########
@@ -1252,6 +1254,8 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
queued_by_job_id = Column(Integer)
pid = Column(Integer)
+ # TODO: Should this be nullable?
+ executor = Column(String(1000))
Review Comment:
Ah right. That makes sense (again)
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2021132962
> BTW. I had no idea the button is there :)
I had never used it and was rather nervous LOL
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2016726203
@o-nikolas can you rebase?
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1525312103
##########
airflow/models/baseoperator.py:
##########
@@ -719,6 +724,8 @@ class derived from this one results in the creation of a task object,
"on_skipped_callback",
"do_xcom_push",
"multiple_outputs",
+ # TODO: should executor go here? I think so
+ "executor",
Review Comment:
> If in doubt, mimic what you have there. _comps is used to calculate hash. if you do not add it here, then two identical tasks with different executor might have same hash. It's certainly wrong no matter what hash is used for. So yes it should be added here.
Yupp, this was my understanding as well. Thanks for the verification!
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2018453616
Yupp, will do, it was all green and no conflicts when I left on Friday :sob: I'll do the rebase today
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2018507597
> > BTW. We discussed in #release-management - that probably we want to hold-on with merging it as we are planning to move the v2-9-test to start from latest `main` rather than cherry-pick changes for 2.9.0b2 - so this one could be merged after we move the branch (cc: @ephraimbuddy ) - as this one adds migration, it makes sense to do in for 2.10 as this is where it will be actually useful.
>
> Okay, that sounds reasonable, when is that branching going to happen? It's helpful to have this in main soon-ish so that I can create more dev branches off of it rather than have several layers of branches based on branches (which is possible of course, but just kind of a pain).
We are waiting for https://github.com/apache/airflow/pull/38446 and one other PR. So probably later today
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2018504298
> BTW. We discussed in #release-management - that probably we want to hold-on with merging it as we are planning to move the v2-9-test to start from latest `main` rather than cherry-pick changes for 2.9.0b2 - so this one could be merged after we move the branch (cc: @ephraimbuddy ) - as this one adds migration, it makes sense to do in for 2.10 as this is where it will be actually useful.
Okay, that sounds reasonable, when is that branching going to happen? It's helpful to have this in main soon-ish so that I can create more dev branches off of it rather than have several layers of branches based on branches (which is possible of course, but just kind of a pain).
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1525325297
##########
airflow/models/taskinstance.py:
##########
@@ -1252,6 +1254,8 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
queued_by_job_id = Column(Integer)
pid = Column(Integer)
+ # TODO: Should this be nullable?
+ executor = Column(String(1000))
Review Comment:
This is actually a nuanced bit that's easy to get confused by. The default must be NULL/empty to maintain the behaviour we have today in the base case. I covered this in the AIP and I think that is the best language I've come up with, so I've attached that below. As well as comment threads from Jens and Jarek who +1 the idea during review:
Explanation from AIP:
Comment thread:
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1524043238
##########
airflow/models/taskinstance.py:
##########
@@ -1252,6 +1254,8 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
queued_by_job_id = Column(Integer)
pid = Column(Integer)
+ # TODO: Should this be nullable?
+ executor = Column(String(1000))
Review Comment:
Yes. Executor should ratehr be not null and historical value shoudl be set to default executor at migration time.
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1524036066
##########
airflow/models/baseoperator.py:
##########
@@ -719,6 +724,8 @@ class derived from this one results in the creation of a task object,
"on_skipped_callback",
"do_xcom_push",
"multiple_outputs",
+ # TODO: should executor go here? I think so
+ "executor",
Review Comment:
If in doubt, mimic what you have there. _comps is used to calculate hash. if you do not add it here, then two identical tasks with different executor might have same hash. It's certainly wrong no matter what hash is used for. So yes it should be added here.
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1530723818
##########
airflow/models/taskinstance.py:
##########
@@ -1252,6 +1254,8 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
queued_by_job_id = Column(Integer)
pid = Column(Integer)
+ # TODO: Should this be nullable?
+ executor = Column(String(1000))
Review Comment:
@potiuk Does the above make sense after I pulled in more context?
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2018990879
Okay @potiuk, @ephraimbuddy
The PR is rebased and green again. Can you merge it when the coast is clear re: 2.9.0 release planning? Thanks!
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2019061460
I have access to the fork/branch he submitted it from, [aws-mwaa:onikolas/aip-61/db_migration*](https://github.com/aws-mwaa/upstream-to-airflow/tree/onikolas/aip-61/db_migration), so I resubmitted it [here](https://github.com/apache/airflow/pull/38474) and verified that it shows his credit on the commits.
* I got that origin fork/branch from here:
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2018467809
BTW. We discussed in #release-management - that probably we want to hold-on with merging it as we are planning to move the v2-9-test to start from latest `main` rather than cherry-pick changes for 2.9.0b2 - so this one could be merged after we move the branch (cc: @ephraimbuddy ) - as this one adds migration, it makes sense to do in for 2.10 as this is where it will be actually useful.
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2019023197
Ups. We were supposed to merge it after we move branch :)
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2019055358
I have reverted my accidental merge here. I don't see a way to reopen this PR so we may need to apply it again later by reverting the revert, unless @o-nikolas opens a new PR?
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2019056738
yep. @o-nikolas to open a PR or you can do it by cherry-picking the reverted commit
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2019042536
Yeah I think it's best now.
BTW. I had no idea the button is there :)
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #38054:
URL: https://github.com/apache/airflow/pull/38054#issuecomment-2019030828
I'm sorry! Want me to revert the commit?
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1525326065
##########
airflow/models/baseoperator.py:
##########
@@ -255,6 +257,7 @@ def partial(
on_retry_callback: None | TaskStateChangeCallback | list[TaskStateChangeCallback] | ArgNotSet = NOTSET,
on_skipped_callback: None | TaskStateChangeCallback | list[TaskStateChangeCallback] | ArgNotSet = NOTSET,
run_as_user: str | None | ArgNotSet = NOTSET,
+ executor: str | None = None,
Review Comment:
Thanks for the details, I will indeed move this to use NOTSET :+1:
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "syedahsn (via GitHub)" <gi...@apache.org>.
syedahsn commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1523598529
##########
airflow/models/abstractoperator.py:
##########
@@ -58,6 +58,8 @@
DEFAULT_OWNER: str = conf.get_mandatory_value("operators", "default_owner")
DEFAULT_POOL_SLOTS: int = 1
DEFAULT_PRIORITY_WEIGHT: int = 1
+# TODO: Is None the right thing here? We want NULL essentially
Review Comment:
Why do we want this to be NULL?
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1520513280
##########
airflow/migrations/versions/0137_2_9_0_add_new_executor_field_to_db.py:
##########
@@ -0,0 +1,48 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""add new executor field to db
+
+Revision ID: 677fdbb7fc54
+Revises: ab34f260b71c
+Create Date: 2024-03-11 15:26:59.186579
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+
+
+# revision identifiers, used by Alembic.
+revision = '677fdbb7fc54'
+down_revision = 'ab34f260b71c'
+branch_labels = None
+depends_on = None
+airflow_version = '2.9.0'
+
+
+def upgrade():
+ """Apply add executor field to task instance"""
+ # NOTE: I cannot find a straightforward sdk doc for SA types. I don't know
+ # if I need default=None. No idea how to tell what are the defaults without docs
Review Comment:
Looking for feedback on this, thanks!
##########
airflow/models/baseoperator.py:
##########
@@ -719,6 +724,8 @@ class derived from this one results in the creation of a task object,
"on_skipped_callback",
"do_xcom_push",
"multiple_outputs",
+ # TODO: should executor go here? I think so
+ "executor",
Review Comment:
Looking for feedback on this, thanks!
##########
airflow/models/taskinstance.py:
##########
@@ -1252,6 +1254,8 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
queued_by_job_id = Column(Integer)
pid = Column(Integer)
+ # TODO: Should this be nullable?
+ executor = Column(String(1000))
Review Comment:
I believe this should be nullable, but just flagging this to be doublechecked in review.
##########
airflow/models/abstractoperator.py:
##########
@@ -58,6 +58,8 @@
DEFAULT_OWNER: str = conf.get_mandatory_value("operators", "default_owner")
DEFAULT_POOL_SLOTS: int = 1
DEFAULT_PRIORITY_WEIGHT: int = 1
+# TODO: Is None the right thing here? We want NULL essentially
Review Comment:
Looking for feedback on this, thanks!
##########
airflow/models/baseoperator.py:
##########
@@ -255,6 +257,7 @@ def partial(
on_retry_callback: None | TaskStateChangeCallback | list[TaskStateChangeCallback] | ArgNotSet = NOTSET,
on_skipped_callback: None | TaskStateChangeCallback | list[TaskStateChangeCallback] | ArgNotSet = NOTSET,
run_as_user: str | None | ArgNotSet = NOTSET,
+ executor: str | None = None,
Review Comment:
I'm just noticing now that others are using NOTSET, instead of None. I'll update this unless anyone else things otherwise.
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1521953578
##########
airflow/models/baseoperator.py:
##########
@@ -255,6 +257,7 @@ def partial(
on_retry_callback: None | TaskStateChangeCallback | list[TaskStateChangeCallback] | ArgNotSet = NOTSET,
on_skipped_callback: None | TaskStateChangeCallback | list[TaskStateChangeCallback] | ArgNotSet = NOTSET,
run_as_user: str | None | ArgNotSet = NOTSET,
+ executor: str | None = None,
Review Comment:
This should use `NOTSET` because we want to fallback to `default_args` when the argument is not used by the user, instead of the default here. This is why we have the `_PARTIAL_DEFAULTS` dict above—the fallback order is
1. User-supplied value
2. The DAG’s `default_args`
3. Value from `_PARTIAL_DEFAULTS`
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1521963038
##########
airflow/models/baseoperator.py:
##########
@@ -858,6 +866,12 @@ def __init__(
if end_date:
self.end_date = timezone.convert_to_utc(end_date)
+ if executor:
+ warnings.warn(
+ "Specifying executors for operators is not yet"
+ f"supported, the value {executor} will have no effect"
Review Comment:
```suggestion
f"supported, the value {executor!r} will have no effect"
```
--
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] Add executor field to the DB and parameter to the operators [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #38054:
URL: https://github.com/apache/airflow/pull/38054#discussion_r1521962702
##########
airflow/models/baseoperator.py:
##########
@@ -719,6 +724,8 @@ class derived from this one results in the creation of a task object,
"on_skipped_callback",
"do_xcom_push",
"multiple_outputs",
+ # TODO: should executor go here? I think so
+ "executor",
Review Comment:
I said this before, the entire dict is pretty sketchy.
--
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