vincbeck commented on code in PR #31786:
URL: https://github.com/apache/airflow/pull/31786#discussion_r1223124523
##########
tests/providers/amazon/aws/hooks/test_ecs.py:
##########
Review Comment:
While you're at it, maybe you can add tests with the option
`start_from_head`? :D
##########
airflow/providers/amazon/aws/hooks/ecs.py:
##########
@@ -168,7 +168,7 @@ def __init__(
self.log_group = log_group
self.log_stream_name = log_stream_name
- self.hook = AwsLogsHook(aws_conn_id=aws_conn_id,
region_name=region_name)
+ self.logs_hook = AwsLogsHook(aws_conn_id=aws_conn_id,
region_name=region_name)
Review Comment:
nit: to make it consistent with other operators, you can define a method
`logs_hook` with @cached_property. That would avoid creating the hook in the
constructor
##########
airflow/providers/amazon/aws/hooks/logs.py:
##########
@@ -103,7 +104,12 @@ def get_log_events(
skip -= event_count
events = []
- yield from events
+ if not start_from_head:
+ # if we are not reading from head, it doesn't make sense to
return events in "normal" order
+ # while hiding the subsequent calls, bc 1-9 queried by batches
of 3 would return 789 456 123
+ yield from reversed(events)
Review Comment:
We can consider it as a bug fix and as such no need to bump the major
version?
--
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]
