Crowiant commented on PR #59097: URL: https://github.com/apache/airflow/pull/59097#issuecomment-3725927163
> Not my area of expertise either (yet) - I've assigned Copilot for an initial review, so please address its commentary as well. However, my 2 cents, if I understand these changes correctly - the `if` block makes the comparison based on a `creation_timestamp`, which is clearly not the `start_time`. Therefore, assigning the `creation_timestamp` to `pod_start_times` in the `if` statement is rather misleading. In that case, why not making the entire comparison based on `creation_timestamp`, replacing the `start_time` completely? Alternatively, if comparison based on `start_time` is useful for most cases, maybe you could introduce a flag for the user so they'll configure the comparison method? > > I'll be happy for an additional review from k8s owners. Hello @shahar1 thank you for your comment! My thought was about extending current logic not changing it completely. In my opinion if start_time happens to be None it should not lead to the error that a user should resolve manually. The recent pod could be defined based on the creation_timestamp. I don´t see it as a different way that a user should define in the operator because at the end the code only specifies which of the identical pods is most recent. WDYT? Also, if it is possible @jscheffl could you please help with the review of the PR? Or advise someone who has expertise to review and approve this 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
