prabhusneha commented on code in PR #46398:
URL: https://github.com/apache/airflow/pull/46398#discussion_r1944441340
##########
airflow/utils/types.py:
##########
@@ -42,7 +43,11 @@ class DagRunType(str, enum.Enum):
def __str__(self) -> str:
return self.value
- def generate_run_id(self, logical_date: datetime) -> str:
+ def generate_run_id(self, logical_date: datetime | None, run_after:
datetime | None) -> str:
+ if logical_date is None:
+ if run_after is None:
+ raise ValueError("run_after cannot be None")
+ return run_after + get_random_string()
return f"{self}__{logical_date.isoformat()}"
Review Comment:
> Maybe if this function should continue to accept one single datetime
value, and we do the if-else check outside instead.
Here we need to know if logical_date is None or not before generating a
random string that gets appended to run_after.
If we take just one argument, it wouldn't know when to append the random
string. Are you suggesting to move this logic into the
callers(Timetable.generate_run_id and DagRun.generate_run_id)?
I went with the current implementation to avoid duplicate code/multiple
function calls.
--
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]
