You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "vandonr-amz (via GitHub)" <gi...@apache.org> on 2023/09/22 22:11:00 UTC
[GitHub] [airflow] vandonr-amz opened a new pull request, #34570: do not fail operator if we cannot find logs
vandonr-amz opened a new pull request, #34570:
URL: https://github.com/apache/airflow/pull/34570
reported by a user on slack, their job succeeds, but the the task fails when trying to compute the link to the logs because their job is not of a "supported type".
This is just a bonus, and there is already code handling the fact that we may have no link to display, so errors shouldn't make the task fail either.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Lee-W commented on a diff in pull request #34570: do not fail operator if we cannot find logs
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #34570:
URL: https://github.com/apache/airflow/pull/34570#discussion_r1335028050
##########
airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -359,7 +359,12 @@ def monitor_job(self, context: Context):
else:
self.hook.wait_for_job(self.job_id)
- awslogs = self.hook.get_job_all_awslogs_info(self.job_id)
+ awslogs = []
+ try:
+ awslogs = self.hook.get_job_all_awslogs_info(self.job_id)
+ except AirflowException as ae:
+ self.log.warning("Cannot determine where to find the AWS logs for this Batch job: %s", ae)
Review Comment:
Should we add a test case to 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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas merged pull request #34570: do not fail operator if we cannot find logs
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas merged PR #34570:
URL: https://github.com/apache/airflow/pull/34570
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #34570: do not fail operator if we cannot find logs
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #34570:
URL: https://github.com/apache/airflow/pull/34570#discussion_r1334878879
##########
airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -359,7 +359,12 @@ def monitor_job(self, context: Context):
else:
self.hook.wait_for_job(self.job_id)
- awslogs = self.hook.get_job_all_awslogs_info(self.job_id)
+ awslogs = []
+ try:
+ awslogs = self.hook.get_job_all_awslogs_info(self.job_id)
+ except AirflowException as ae:
+ self.log.warning("cannot determine where to find aws logs for this batch job: %s", ae)
Review Comment:
I think this is happen because we lack of support AWS Batch on EKS in the BatchOperator.
So it might work on simple case and fail on logs but it general it could fail in other places too.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #34570: do not fail operator if we cannot find logs
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #34570:
URL: https://github.com/apache/airflow/pull/34570#discussion_r1334855291
##########
airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -359,7 +359,12 @@ def monitor_job(self, context: Context):
else:
self.hook.wait_for_job(self.job_id)
- awslogs = self.hook.get_job_all_awslogs_info(self.job_id)
+ awslogs = []
+ try:
+ awslogs = self.hook.get_job_all_awslogs_info(self.job_id)
+ except AirflowException as ae:
+ self.log.warning("cannot determine where to find aws logs for this batch job: %s", ae)
Review Comment:
```suggestion
self.log.warning("Cannot determine where to find the AWS logs for this Batch job: %s", ae)
```
Also are we sure all cases of AirflowException are safe to catch and warn on here? Or should we check the message to ensure it's the specific case you're describing in the log message?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #34570: do not fail operator if we cannot find logs
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #34570:
URL: https://github.com/apache/airflow/pull/34570#discussion_r1334857426
##########
airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -359,7 +359,12 @@ def monitor_job(self, context: Context):
else:
self.hook.wait_for_job(self.job_id)
- awslogs = self.hook.get_job_all_awslogs_info(self.job_id)
+ awslogs = []
+ try:
+ awslogs = self.hook.get_job_all_awslogs_info(self.job_id)
+ except AirflowException as ae:
+ self.log.warning("cannot determine where to find aws logs for this batch job: %s", ae)
Review Comment:
the purpose of this function we're calling is to get the info about where the logs went. So this message should cover _any_ failure in this method.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #34570: do not fail operator if we cannot find logs
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #34570:
URL: https://github.com/apache/airflow/pull/34570#discussion_r1336130248
##########
airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -359,7 +359,12 @@ def monitor_job(self, context: Context):
else:
self.hook.wait_for_job(self.job_id)
- awslogs = self.hook.get_job_all_awslogs_info(self.job_id)
+ awslogs = []
+ try:
+ awslogs = self.hook.get_job_all_awslogs_info(self.job_id)
+ except AirflowException as ae:
+ self.log.warning("Cannot determine where to find the AWS logs for this Batch job: %s", ae)
Review Comment:
honestly, I think a test for this would be overkill.
It's a 6 lines change, and there is really very little room for a mistake in it, there is no logic, it's just a try/except.
Adding a test would make this commit 10x bigger, with no added benefit imho.
It's not like someone would come across this code and remove the try/except block thinking it's useless, only to be caught by the test. It's already documented in place by the text of the warning log.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Lee-W commented on a diff in pull request #34570: do not fail operator if we cannot find logs
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #34570:
URL: https://github.com/apache/airflow/pull/34570#discussion_r1336498643
##########
airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -359,7 +359,12 @@ def monitor_job(self, context: Context):
else:
self.hook.wait_for_job(self.job_id)
- awslogs = self.hook.get_job_all_awslogs_info(self.job_id)
+ awslogs = []
+ try:
+ awslogs = self.hook.get_job_all_awslogs_info(self.job_id)
+ except AirflowException as ae:
+ self.log.warning("Cannot determine where to find the AWS logs for this Batch job: %s", ae)
Review Comment:
sounds good 👍
##########
airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -359,7 +359,12 @@ def monitor_job(self, context: Context):
else:
self.hook.wait_for_job(self.job_id)
- awslogs = self.hook.get_job_all_awslogs_info(self.job_id)
+ awslogs = []
+ try:
+ awslogs = self.hook.get_job_all_awslogs_info(self.job_id)
+ except AirflowException as ae:
+ self.log.warning("Cannot determine where to find the AWS logs for this Batch job: %s", ae)
Review Comment:
sounds good 👍
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org