Re: [PR] Updated logic to allow AWS Batch Hook `get_job_description` retries to be more effective [airflow]
potiuk merged PR #38998: URL: https://github.com/apache/airflow/pull/38998 -- 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] Updated logic to allow AWS Batch Hook `get_job_description` retries to be more effective [airflow]
shahar1 commented on code in PR #38998: URL: https://github.com/apache/airflow/pull/38998#discussion_r1573340312 ## airflow/providers/amazon/aws/hooks/batch_client.py: ## @@ -396,7 +396,11 @@ def get_job_description(self, job_id: str) -> dict: try: response = self.get_conn().describe_jobs(jobs=[job_id]) -return self.parse_job_description(job_id, response) +job_description = self.parse_job_description(job_id, response) +# allow us to retry getting the job description in case +# we called it before AWS could register the job +if job_description: +return job_description Review Comment: Good call, done! -- 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] Updated logic to allow AWS Batch Hook `get_job_description` retries to be more effective [airflow]
o-nikolas commented on code in PR #38998:
URL: https://github.com/apache/airflow/pull/38998#discussion_r1566178573
##
airflow/providers/amazon/aws/hooks/batch_client.py:
##
@@ -396,7 +396,11 @@ def get_job_description(self, job_id: str) -> dict:
try:
response = self.get_conn().describe_jobs(jobs=[job_id])
-return self.parse_job_description(job_id, response)
+job_description = self.parse_job_description(job_id, response)
+# allow us to retry getting the job description in case
+# we called it before AWS could register the job
+if job_description:
+return job_description
Review Comment:
As mentioned below, instead of changing the spec of the below function to
now return None, just catch the AirflowException here and retry on that.
##
airflow/providers/amazon/aws/hooks/batch_client.py:
##
@@ -414,22 +418,20 @@ def get_job_description(self, job_id: str) -> dict:
)
@staticmethod
-def parse_job_description(job_id: str, response: dict) -> dict:
+def parse_job_description(job_id: str, response: dict) -> dict | None:
"""
Parse job description to extract description for job_id.
:param job_id: a Batch job ID
:param response: an API response for describe jobs
-:return: an API response to describe job_id
-
-:raises: AirflowException
+:return: an API response to describe job_id or None if job_id not
found in response
"""
jobs = response.get("jobs", [])
matching_jobs = [job for job in jobs if job.get("jobId") == job_id]
if len(matching_jobs) != 1:
-raise AirflowException(f"AWS Batch job ({job_id}) description
error: response: {response}")
Review Comment:
Why not just catch the exception in the calling function
(`get_job_description`) instead of changing the signature of this function? I
think that would be a lot cleaner.
--
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] Updated logic to allow AWS Batch Hook `get_job_description` retries to be more effective [airflow]
vincbeck commented on PR #38998: URL: https://github.com/apache/airflow/pull/38998#issuecomment-2057092799 @syedahsn -- 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] Updated logic to allow AWS Batch Hook `get_job_description` retries to be more effective [airflow]
dirrao commented on code in PR #38998:
URL: https://github.com/apache/airflow/pull/38998#discussion_r1564401980
##
airflow/providers/amazon/aws/hooks/batch_client.py:
##
@@ -414,22 +418,20 @@ def get_job_description(self, job_id: str) -> dict:
)
@staticmethod
-def parse_job_description(job_id: str, response: dict) -> dict:
+def parse_job_description(job_id: str, response: dict) -> dict | None:
"""
Parse job description to extract description for job_id.
:param job_id: a Batch job ID
:param response: an API response for describe jobs
-:return: an API response to describe job_id
-
-:raises: AirflowException
+:return: an API response to describe job_id or None if job_id not
found in response
"""
jobs = response.get("jobs", [])
matching_jobs = [job for job in jobs if job.get("jobId") == job_id]
if len(matching_jobs) != 1:
-raise AirflowException(f"AWS Batch job ({job_id}) description
error: response: {response}")
Review Comment:
how are we handling the case where the job id itself doesn't exist at all.
are we failing the job?
--
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
[PR] Updated logic to allow AWS Batch Hook `get_job_description` retries to be more effective [airflow]
shahar1 opened a new pull request, #38998:
URL: https://github.com/apache/airflow/pull/38998
related: #37552 (stale PR)
---
**^ Add meaningful description above**
Read the **[Pull Request
Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)**
for more information.
In case of fundamental code changes, an Airflow Improvement Proposal
([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals))
is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party
License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a
newsfragment file, named `{pr_number}.significant.rst` or
`{issue_number}.significant.rst`, in
[newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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] Updated logic to allow AWS Batch Hook `get_job_description` retries to be more effective [airflow]
github-actions[bot] closed pull request #37552: Updated logic to allow AWS Batch Hook `get_job_description` retries to be more effective URL: https://github.com/apache/airflow/pull/37552 -- 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] Updated logic to allow AWS Batch Hook `get_job_description` retries to be more effective [airflow]
github-actions[bot] commented on PR #37552: URL: https://github.com/apache/airflow/pull/37552#issuecomment-2040804907 This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. -- 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] Updated logic to allow AWS Batch Hook `get_job_description` retries to be more effective [airflow]
Taragolis commented on code in PR #37552: URL: https://github.com/apache/airflow/pull/37552#discussion_r1495435540 ## airflow/providers/amazon/aws/hooks/batch_client.py: ## @@ -395,7 +395,13 @@ def get_job_description(self, job_id: str) -> dict: try: response = self.get_conn().describe_jobs(jobs=[job_id]) -return self.parse_job_description(job_id, response) +job_description = self.parse_job_description(job_id, response) +# allow us to retry getting the job description in case +# we called it before AWS could register the job +if job_description: +return job_description +else: +continue Review Comment: Seem like this continue redundant -- 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
