potiuk commented on code in PR #30308:
URL: https://github.com/apache/airflow/pull/30308#discussion_r1163039124
##########
airflow/jobs/job.py:
##########
@@ -292,3 +281,51 @@ def most_recent_job(job_type: str, session: Session =
NEW_SESSION) -> Job | None
)
.first()
)
+
+
+@provide_session
+def prepare_for_execution(job: Job | JobPydantic, session: Session =
NEW_SESSION):
Review Comment:
Hey @uranusjr - I think I've figured a good way that address your concerns.
I just pushed a slightly re-worked version that avoids instance checks.
I still keep `Job | JobPydantic` type hint to make it easy to continue
working on AIP-44 but at the same time i added __ENABLE_AIP_44 feature branch
conditions, renamed methods that are specific to aip-44 to contain the aip_44
in the name (and raise Runtime Error if used when it is disabled), and some
extra asserts to make sure we are not passing JobPydantic anyewhere in our
tests when AIP-44 is disabled.
This sis a bit more ugly (IMHO), but also a bit safer for cherry-picking and
avoids the (unnecessary for now) check for the type which makes it both faster
and safer. Whe _ENABLE_AIP_44 is not set, the only pth is a quick boolean check
for the value and all the rest of the code is executed directly rather through
the new private methods. When we enable AIP_44 on the other hand, we will go
the more "type-checked" route (and in the future we might figure out better way
to do it - once we convert the methods to @internal_api, we might decide to
split the logic a bit earliier.
Also the AIP-44 methods are "below" regular ones, so it will be easy to
avoid conflicts and confusion which ones are used
Sounds like best of both worlds.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
