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 2024/04/23 04:10:02 UTC
[PR] Provide mechanism to serialize airflow BaseTrigger object [airflow]
dstandish opened a new pull request, #39185:
URL: https://github.com/apache/airflow/pull/39185
It is needed for AIP-44 when we pass the trigger object attached to the TaskDeferred exception object.
--
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] Provide mechanism to serialize airflow BaseTrigger object [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish merged PR #39185:
URL: https://github.com/apache/airflow/pull/39185
--
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] Provide mechanism to serialize airflow BaseTrigger object [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #39185:
URL: https://github.com/apache/airflow/pull/39185#discussion_r1575667957
##########
airflow/serialization/enums.py:
##########
@@ -44,6 +44,7 @@ class DagAttributeTypes(str, Enum):
TIMEDELTA = "timedelta"
TIMEZONE = "timezone"
RELATIVEDELTA = "relativedelta"
+ BASE_TRIGGER = "base_trigger"
Review Comment:
ok. 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] Provide mechanism to serialize airflow BaseTrigger object [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #39185:
URL: https://github.com/apache/airflow/pull/39185#discussion_r1575631513
##########
airflow/serialization/enums.py:
##########
@@ -44,6 +44,7 @@ class DagAttributeTypes(str, Enum):
TIMEDELTA = "timedelta"
TIMEZONE = "timezone"
RELATIVEDELTA = "relativedelta"
+ BASE_TRIGGER = "base_trigger"
Review Comment:
How about just call this `trigger`?
--
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] Provide mechanism to serialize airflow BaseTrigger object [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #39185:
URL: https://github.com/apache/airflow/pull/39185#discussion_r1575664046
##########
airflow/serialization/enums.py:
##########
@@ -44,6 +44,7 @@ class DagAttributeTypes(str, Enum):
TIMEDELTA = "timedelta"
TIMEZONE = "timezone"
RELATIVEDELTA = "relativedelta"
+ BASE_TRIGGER = "base_trigger"
Review Comment:
Well... i put this to you. Suppose we _did_ need a type for the ORM. what would be your call in that case?
I think it's best to just assume we do need it and name given that assumption, even though not currently true.
Then if we ever do need it we won't have an issue.
I don't veto calling it the non-orm one `trigger`. We could also call it `trigger_non_orm`. Or we could just plan to call the orm one `trigger_orm`.
--
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] Provide mechanism to serialize airflow BaseTrigger object [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #39185:
URL: https://github.com/apache/airflow/pull/39185#discussion_r1575659970
##########
airflow/serialization/enums.py:
##########
@@ -44,6 +44,7 @@ class DagAttributeTypes(str, Enum):
TIMEDELTA = "timedelta"
TIMEZONE = "timezone"
RELATIVEDELTA = "relativedelta"
+ BASE_TRIGGER = "base_trigger"
Review Comment:
The thing is, I feel we should call this `TRIGGER` regardless of the class name since we don’t need to have a type for the ORM class (at least I don’t think so? Do we?)
--
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] Provide mechanism to serialize airflow BaseTrigger object [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #39185:
URL: https://github.com/apache/airflow/pull/39185#discussion_r1575632104
##########
airflow/serialization/enums.py:
##########
@@ -44,6 +44,7 @@ class DagAttributeTypes(str, Enum):
TIMEDELTA = "timedelta"
TIMEZONE = "timezone"
RELATIVEDELTA = "relativedelta"
+ BASE_TRIGGER = "base_trigger"
Review Comment:
because there are two trigger classes. BaseTrigger is non-orm and Trigger is orm.
--
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] Provide mechanism to serialize airflow BaseTrigger object [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #39185:
URL: https://github.com/apache/airflow/pull/39185#discussion_r1575632362
##########
airflow/serialization/enums.py:
##########
@@ -44,6 +44,7 @@ class DagAttributeTypes(str, Enum):
TIMEDELTA = "timedelta"
TIMEZONE = "timezone"
RELATIVEDELTA = "relativedelta"
+ BASE_TRIGGER = "base_trigger"
Review Comment:
BaseTrigger->Trigger is kindof like DAG->DagModel
--
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] Provide mechanism to serialize airflow BaseTrigger object [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #39185:
URL: https://github.com/apache/airflow/pull/39185#discussion_r1575649215
##########
airflow/serialization/enums.py:
##########
@@ -44,6 +44,7 @@ class DagAttributeTypes(str, Enum):
TIMEDELTA = "timedelta"
TIMEZONE = "timezone"
RELATIVEDELTA = "relativedelta"
+ BASE_TRIGGER = "base_trigger"
Review Comment:
i agree on the naming. We don't _yet_...... afaik......
--
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] Provide mechanism to serialize airflow BaseTrigger object [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #39185:
URL: https://github.com/apache/airflow/pull/39185#discussion_r1575632362
##########
airflow/serialization/enums.py:
##########
@@ -44,6 +44,7 @@ class DagAttributeTypes(str, Enum):
TIMEDELTA = "timedelta"
TIMEZONE = "timezone"
RELATIVEDELTA = "relativedelta"
+ BASE_TRIGGER = "base_trigger"
Review Comment:
BaseTrigger->Trigger is kindof like DAG->DagModel
--
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] Provide mechanism to serialize airflow BaseTrigger object [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #39185:
URL: https://github.com/apache/airflow/pull/39185#discussion_r1575666282
##########
airflow/serialization/enums.py:
##########
@@ -44,6 +44,7 @@ class DagAttributeTypes(str, Enum):
TIMEDELTA = "timedelta"
TIMEZONE = "timezone"
RELATIVEDELTA = "relativedelta"
+ BASE_TRIGGER = "base_trigger"
Review Comment:
We can call it `trigger_model` like `dag_model`.
Thinking a bit, calling this `base_trigger` for now is probably not a bad idea as long as we don’t call the ORM class `trigger` (we can use `trigger_model` instead).
--
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] Provide mechanism to serialize airflow BaseTrigger object [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #39185:
URL: https://github.com/apache/airflow/pull/39185#discussion_r1575664046
##########
airflow/serialization/enums.py:
##########
@@ -44,6 +44,7 @@ class DagAttributeTypes(str, Enum):
TIMEDELTA = "timedelta"
TIMEZONE = "timezone"
RELATIVEDELTA = "relativedelta"
+ BASE_TRIGGER = "base_trigger"
Review Comment:
Well... i put this to you. Suppose we _did_ need a type for the ORM. what would be your call in that case?
I think it's best to just assume we do need it and name given that assumption, even though not currently true.
Then if we ever do need it we won't have an issue.
I don't veto calling it the non-orm one `trigger`. We could call it `trigger_non_orm`. Or we could just plan to call the orm one `trigger_orm`.
--
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] Provide mechanism to serialize airflow BaseTrigger object [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #39185:
URL: https://github.com/apache/airflow/pull/39185#discussion_r1575634883
##########
airflow/serialization/enums.py:
##########
@@ -44,6 +44,7 @@ class DagAttributeTypes(str, Enum):
TIMEDELTA = "timedelta"
TIMEZONE = "timezone"
RELATIVEDELTA = "relativedelta"
+ BASE_TRIGGER = "base_trigger"
Review Comment:
Ah OK. Do we need to be able to serialise `Trigger` too? (The naming is a bit confusing too fwiw, we use Foo and FooModel in most places instead, but this is not the time to change 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