You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "rawwar (via GitHub)" <gi...@apache.org> on 2025/02/21 07:06:34 UTC
[PR] add startTime to paginator.paginate when fetching logs in GlueJobHook [airflow]
rawwar opened a new pull request, #46950:
URL: https://github.com/apache/airflow/pull/46950
fixes #44694
--
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
Re: [PR] add startTime to paginator.paginate when fetching logs in GlueJobHook [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on PR #46950:
URL: https://github.com/apache/airflow/pull/46950#issuecomment-2683648972
@ferruzzi , can we merge this?
--
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
Re: [PR] add startTime to paginator.paginate when fetching logs in GlueJobHook [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #46950:
URL: https://github.com/apache/airflow/pull/46950#discussion_r1968437877
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -245,6 +246,9 @@ def display_logs_from(log_group: str, continuation_token: str | None) -> str | N
for response in paginator.paginate(
logGroupName=log_group,
logStreamNames=[run_id],
+ startTime=int(
+ (datetime.now() - timedelta(hours=24)).timestamp() * 1000
+ ), # 24 hours ago in milliseconds
Review Comment:
Also, entirely unrelated to this PR, but while looking to see if we have a get_job_run variant in the hook I noticed this oddity:
In the Glue hook [we implement list_jobs](list) which just calls and returns `get_jobs()`, but the API **_has_** a [list_jobs call](https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/glue/client/list_jobs.html)..... we should maybe look into untangling that. It looks like get_jobs is returning a list of full details for each job, what I would expect to see from a describe_jobs() - which doesn't exist - but the API's version of list_jobs is just returning a list of names, so hat would be a breaking change...
IMHO (unless there is some context I'm missing, of course) having `hook.conn.list_jobs()` return something entirely different than `hook.list_jobs()` is a bad idea. Maybe we should deprecate our hook.list_jobs() and either rename it hook.describe_jobs() or, preferably, drop it entirely since it's just a thin-wrapper renaming an existing api call. It's a bad habit on our side to rename an existing provider's API feature like that.
That's not part of this PR, but maybe something to do next if you want to poke at the Glue stuff more.
--
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
Re: [PR] add startTime to paginator.paginate when fetching logs in GlueJobHook [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #46950:
URL: https://github.com/apache/airflow/pull/46950#discussion_r1968850050
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -245,6 +246,9 @@ def display_logs_from(log_group: str, continuation_token: str | None) -> str | N
for response in paginator.paginate(
logGroupName=log_group,
logStreamNames=[run_id],
+ startTime=int(
+ (datetime.now() - timedelta(hours=24)).timestamp() * 1000
+ ), # 24 hours ago in milliseconds
Review Comment:
+1 to that. I'll create an issue and work on that
--
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
Re: [PR] add startTime to paginator.paginate when fetching logs in GlueJobHook [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #46950:
URL: https://github.com/apache/airflow/pull/46950#discussion_r1968896660
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -270,7 +274,7 @@ def display_logs_from(log_group: str, continuation_token: str | None) -> str | N
self.log.info("No new log from the Glue Job in %s", log_group)
return next_token
- log_group_prefix = self.conn.get_job_run(JobName=job_name, RunId=run_id)["JobRun"]["LogGroupName"]
+ log_group_prefix = job_run["LogGroupName"]
Review Comment:
I was thinking of making it a property. But, I'll see how often its used across and do refactoring as part of another 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add startTime to paginator.paginate when fetching logs in GlueJobHook [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi merged PR #46950:
URL: https://github.com/apache/airflow/pull/46950
--
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
Re: [PR] add startTime to paginator.paginate when fetching logs in GlueJobHook [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #46950:
URL: https://github.com/apache/airflow/pull/46950#discussion_r1968888293
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -270,7 +274,7 @@ def display_logs_from(log_group: str, continuation_token: str | None) -> str | N
self.log.info("No new log from the Glue Job in %s", log_group)
return next_token
- log_group_prefix = self.conn.get_job_run(JobName=job_name, RunId=run_id)["JobRun"]["LogGroupName"]
+ log_group_prefix = job_run["LogGroupName"]
Review Comment:
Ah, nice, I'm glad you can make that one call multi-purpose down here too. That works out nicely.
--
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
Re: [PR] add startTime to paginator.paginate when fetching logs in GlueJobHook [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #46950:
URL: https://github.com/apache/airflow/pull/46950#discussion_r1968886492
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -236,6 +236,9 @@ def print_job_logs(
"""
log_client = self.logs_hook.get_conn()
paginator = log_client.get_paginator("filter_log_events")
+ job_run = self.conn.get_job_run(JobName=job_name, RunId=run_id)["JobRun"]
+ # StartTime needs to be an int and is Epoch time in milliseconds
Review Comment:
```suggestion
# StartTime needs to be an int and is Epoch time in milliseconds
```
--
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
Re: [PR] add startTime to paginator.paginate when fetching logs in GlueJobHook [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #46950:
URL: https://github.com/apache/airflow/pull/46950#discussion_r1968426262
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -245,6 +246,9 @@ def display_logs_from(log_group: str, continuation_token: str | None) -> str | N
for response in paginator.paginate(
logGroupName=log_group,
logStreamNames=[run_id],
+ startTime=int(
+ (datetime.now() - timedelta(hours=24)).timestamp() * 1000
+ ), # 24 hours ago in milliseconds
Review Comment:
In the other thread you mentioned hardcoding the start time of the task. Sorry I didn't get back to you sooner. That sounds like a good plan to me. If that isn't possible, I guess this works. You have the job_name and run_id, I suspect you can get it from those, something like
`glue_hook.conn.get_job_run(job_name, run_id)['JobRun']['StartedOn']`
((docs [here](https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/glue/client/get_job_run.html)))
--
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
Re: [PR] add startTime to paginator.paginate when fetching logs in GlueJobHook [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #46950:
URL: https://github.com/apache/airflow/pull/46950#discussion_r1966433770
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -245,6 +246,9 @@ def display_logs_from(log_group: str, continuation_token: str | None) -> str | N
for response in paginator.paginate(
logGroupName=log_group,
logStreamNames=[run_id],
+ startTime=int(
+ (datetime.now() - timedelta(hours=24)).timestamp() * 1000
+ ), # 24 hours ago in milliseconds
Review Comment:
We can definitely have an instance variable within hook, initialized with the time of hook creation, that we can use to across as a startTime. Not really sure if we should hard code this. But, open to ideas.
--
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
Re: [PR] add startTime to paginator.paginate when fetching logs in GlueJobHook [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #46950:
URL: https://github.com/apache/airflow/pull/46950#discussion_r1967949229
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -245,6 +246,9 @@ def display_logs_from(log_group: str, continuation_token: str | None) -> str | N
for response in paginator.paginate(
logGroupName=log_group,
logStreamNames=[run_id],
+ startTime=int(
+ (datetime.now() - timedelta(hours=24)).timestamp() * 1000
+ ), # 24 hours ago in milliseconds
Review Comment:
So if theoretically a job last more than 24 hours, you'll miss some logs? Not sure if this is a real use case, I am just asking :)
--
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
Re: [PR] add startTime to paginator.paginate when fetching logs in GlueJobHook [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #46950:
URL: https://github.com/apache/airflow/pull/46950#discussion_r1968426262
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -245,6 +246,9 @@ def display_logs_from(log_group: str, continuation_token: str | None) -> str | N
for response in paginator.paginate(
logGroupName=log_group,
logStreamNames=[run_id],
+ startTime=int(
+ (datetime.now() - timedelta(hours=24)).timestamp() * 1000
+ ), # 24 hours ago in milliseconds
Review Comment:
In the other thread you mentioned hardcoding the start time of the task. Sorry I didn't get back to you sooner. That sounds like a good plan to me. If that isn't possible, I guess this works. You have the job_name and run_id, I suspect you can get it from those, something like
`glue_hook.conn.get_job_run(job_name, run_id)['JobRun']['StatedOn']`
((docs [here](https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/glue/client/get_job_run.html)))
--
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
Re: [PR] add startTime to paginator.paginate when fetching logs in GlueJobHook [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #46950:
URL: https://github.com/apache/airflow/pull/46950#discussion_r1968850050
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -245,6 +246,9 @@ def display_logs_from(log_group: str, continuation_token: str | None) -> str | N
for response in paginator.paginate(
logGroupName=log_group,
logStreamNames=[run_id],
+ startTime=int(
+ (datetime.now() - timedelta(hours=24)).timestamp() * 1000
+ ), # 24 hours ago in milliseconds
Review Comment:
+1 to that. I'll create an issue and work on that
Edit: created it - https://github.com/apache/airflow/issues/47046
--
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
Re: [PR] add startTime to paginator.paginate when fetching logs in GlueJobHook [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #46950:
URL: https://github.com/apache/airflow/pull/46950#discussion_r1966433770
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -245,6 +246,9 @@ def display_logs_from(log_group: str, continuation_token: str | None) -> str | N
for response in paginator.paginate(
logGroupName=log_group,
logStreamNames=[run_id],
+ startTime=int(
+ (datetime.now() - timedelta(hours=24)).timestamp() * 1000
+ ), # 24 hours ago in milliseconds
Review Comment:
We can definitely have an instance variable within hook that we can use to across as a startTime. Not really sure if we should hard code this. But, open to ideas.
--
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