Re: [PR] Fix try_number handling when db isolation enabled [airflow]
dstandish merged PR #38943: URL: https://github.com/apache/airflow/pull/38943 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix try_number handling when db isolation enabled [airflow]
potiuk commented on code in PR #38943: URL: https://github.com/apache/airflow/pull/38943#discussion_r1562477057 ## airflow/models/taskinstance.py: ## @@ -539,7 +539,7 @@ def _refresh_from_db( task_instance.end_date = ti.end_date task_instance.duration = ti.duration task_instance.state = ti.state -task_instance.try_number = ti._try_number # private attr to get value unaltered by accessor +task_instance.try_number = _get_private_try_number(task_instance=ti) Review Comment: I think there were at least one or two attempts I remember in the past to remove it - and failed so far. I believe there is race conditions that try number is increased in the DB when task state is changed from RUNNING to FAILED (for example when user stops the task from the UI) and try_number is already stored in the DB - but the running task might still be running and stopping itself, so it should continue logging to the same try_number it was started with. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix try_number handling when db isolation enabled [airflow]
dstandish commented on code in PR #38943: URL: https://github.com/apache/airflow/pull/38943#discussion_r1562051588 ## airflow/models/taskinstance.py: ## @@ -539,7 +539,7 @@ def _refresh_from_db( task_instance.end_date = ti.end_date task_instance.duration = ti.duration task_instance.state = ti.state -task_instance.try_number = ti._try_number # private attr to get value unaltered by accessor +task_instance.try_number = _get_private_try_number(task_instance=ti) Review Comment: According to the comments it is just “to ensure logs go to right file “ but if that’s really all, it could be corrected in the file task handler and try number could just be one value -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix try_number handling when db isolation enabled [airflow]
uranusjr commented on code in PR #38943: URL: https://github.com/apache/airflow/pull/38943#discussion_r1562050194 ## airflow/models/taskinstance.py: ## @@ -539,7 +539,7 @@ def _refresh_from_db( task_instance.end_date = ti.end_date task_instance.duration = ti.duration task_instance.state = ti.state -task_instance.try_number = ti._try_number # private attr to get value unaltered by accessor +task_instance.try_number = _get_private_try_number(task_instance=ti) Review Comment: I don’t know either (never fully understood the logic tbh) -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix try_number handling when db isolation enabled [airflow]
uranusjr commented on code in PR #38943: URL: https://github.com/apache/airflow/pull/38943#discussion_r1562050194 ## airflow/models/taskinstance.py: ## @@ -539,7 +539,7 @@ def _refresh_from_db( task_instance.end_date = ti.end_date task_instance.duration = ti.duration task_instance.state = ti.state -task_instance.try_number = ti._try_number # private attr to get value unaltered by accessor +task_instance.try_number = _get_private_try_number(task_instance=ti) Review Comment: I don’t know either (never fully understood the logic tbh). Talking to Ash may help. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix try_number handling when db isolation enabled [airflow]
dstandish commented on code in PR #38943: URL: https://github.com/apache/airflow/pull/38943#discussion_r1562047896 ## airflow/models/dagrun.py: ## @@ -650,7 +650,6 @@ def get_task_instance( ) @staticmethod -@internal_api_call Review Comment: Basis for conclusion is, I can run task without 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix try_number handling when db isolation enabled [airflow]
dstandish commented on code in PR #38943: URL: https://github.com/apache/airflow/pull/38943#discussion_r1562046802 ## airflow/models/taskinstance.py: ## @@ -539,7 +539,7 @@ def _refresh_from_db( task_instance.end_date = ti.end_date task_instance.duration = ti.duration task_instance.state = ti.state -task_instance.try_number = ti._try_number # private attr to get value unaltered by accessor +task_instance.try_number = _get_private_try_number(task_instance=ti) Review Comment: Yes this problem should go away completely I actually doubt we really need these shenanigans now (specifically the conditional +1 logic) -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix try_number handling when db isolation enabled [airflow]
uranusjr commented on code in PR #38943: URL: https://github.com/apache/airflow/pull/38943#discussion_r1562045467 ## airflow/models/taskinstance.py: ## @@ -539,7 +539,7 @@ def _refresh_from_db( task_instance.end_date = ti.end_date task_instance.duration = ti.duration task_instance.state = ti.state -task_instance.try_number = ti._try_number # private attr to get value unaltered by accessor +task_instance.try_number = _get_private_try_number(task_instance=ti) Review Comment: I wonder if this can be improved when we implement TaskInstanceRun… -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix try_number handling when db isolation enabled [airflow]
uranusjr commented on code in PR #38943: URL: https://github.com/apache/airflow/pull/38943#discussion_r1562044901 ## airflow/models/dagrun.py: ## @@ -650,7 +650,6 @@ def get_task_instance( ) @staticmethod -@internal_api_call Review Comment: cc @vincbeck -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix try_number handling when db isolation enabled [airflow]
dstandish commented on PR #38943: URL: https://github.com/apache/airflow/pull/38943#issuecomment-2050592083 this, dealing, as it does, with try_number, is a weird one, so i'm gonna let it go overnight to see if @uranusjr @potiuk @jedcunningham want to give it a look -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix try_number handling when db isolation enabled [airflow]
dstandish commented on code in PR #38943: URL: https://github.com/apache/airflow/pull/38943#discussion_r1561498670 ## airflow/models/dagrun.py: ## @@ -650,7 +650,6 @@ def get_task_instance( ) @staticmethod -@internal_api_call Review Comment: this is a drive-by it doesn't need to be RPC AFAICT -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
