shahar1 commented on PR #59097: URL: https://github.com/apache/airflow/pull/59097#issuecomment-3733215973
> > 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? > Luckily I've started recently working on a CKA certification, so I feel more comfortable reviewing this PR. Please avoid tagging other commiters/PMC from now - let's try settling this one between us. If anyone else wants to review or comment, they are more than welcome, of course. For future PRs, please be aware of matter - as Jarek said, arbitrarily tagging maintainers might "taint" your current and future PRs from review (k8s pun intended). For better visibility in future PRs, I recommend sending a message in the `#contributors` Slack channel. Now, let's get to the what needs to be done: 1. I closed irrelevant/nitpicking comments by Copilot - I left only the one which I find relevant (unreachable `else` statement). 2. I understood the fallback logic, but it should be **clearly** reflected to the user (probably by `log.info`), and also documented shortly in the function's docstring. Please take care of the two comments above, and from my prespective it will be good to merge. Thank you and good luck! -- 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]
