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/06/09 21:53:40 UTC
[GitHub] [airflow] vandonr-amz opened a new pull request, #31824: Use a continuation token to get logs in ecs
vandonr-amz opened a new pull request, #31824:
URL: https://github.com/apache/airflow/pull/31824
The current implem browses logs from the start every time it tries to get new logs, which results in longer and longer fetching times, and also many calls that can potentially trigger rate limits.
There is a very simple way to avoid this, with the token returned by every request that allows to continue from where the last call left.
--
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 #31824: Use a continuation token to get logs in ecs
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas merged PR #31824:
URL: https://github.com/apache/airflow/pull/31824
--
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 #31824: Use a continuation token to get logs in ecs
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #31824:
URL: https://github.com/apache/airflow/pull/31824#discussion_r1225619320
##########
airflow/providers/amazon/aws/hooks/logs.py:
##########
@@ -63,7 +63,7 @@ def get_log_events(
start_time: int = 0,
skip: int = 0,
start_from_head: bool = True,
- continuation_token: ContinuationToken = ContinuationToken(),
+ continuation_token: ContinuationToken | None = None,
Review Comment:
with the previous version, I didn't realize that the "default object" is created only once. And since we modify it in the method, the default object is modified, and when this method is called again with the default parameters, instead of getting a clean object, we get the modified one from the last call.
--
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 #31824: Use a continuation token to get logs in ecs
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #31824:
URL: https://github.com/apache/airflow/pull/31824#discussion_r1225596631
##########
airflow/providers/amazon/aws/hooks/logs.py:
##########
@@ -113,16 +113,16 @@ def get_log_events(
yield from events
+ if continuation_token.value == response["nextForwardToken"]:
+ return
+
if not event_count:
num_consecutive_empty_response += 1
if num_consecutive_empty_response >= NUM_CONSECUTIVE_EMPTY_RESPONSE_EXIT_THRESHOLD:
# Exit if there are more than NUM_CONSECUTIVE_EMPTY_RESPONSE_EXIT_THRESHOLD consecutive
# empty responses
return
- elif continuation_token.value != response["nextForwardToken"]:
- num_consecutive_empty_response = 0
Review Comment:
doing this check after the check on `not event_count` means that even when we get the same token twice in a row, clearly indicating that there is nothing more to read, we still do `NUM_CONSECUTIVE_EMPTY_RESPONSE_EXIT_THRESHOLD` (=3) calls before returning.
Moving the check on token above saves 2 calls every time we get the correct termination condition from cloudwatch.
--
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 #31824: Use a continuation token to get logs in ecs
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #31824:
URL: https://github.com/apache/airflow/pull/31824#discussion_r1224895736
##########
airflow/providers/amazon/aws/hooks/logs.py:
##########
@@ -72,16 +79,17 @@ def get_log_events(
This is for when there are multiple entries at the same timestamp.
:param start_from_head: whether to start from the beginning (True) of the log or
at the end of the log (False).
+ :param continuation_token: a wrapper around a token indicating where to read logs from.
Review Comment:
```suggestion
:param continuation_token: a token indicating where to read logs from.
```
--
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 pull request #31824: Use a continuation token to get logs in ecs
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on PR #31824:
URL: https://github.com/apache/airflow/pull/31824#issuecomment-1585723846
> Did you do any manual tests to ensure all the logs are coming through like you expect?
😬
I will today. I need to tweak a bit the system test to last long enough to have several polls of the logs.
--
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 pull request #31824: Use a continuation token to get logs in ecs
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on PR #31824:
URL: https://github.com/apache/airflow/pull/31824#issuecomment-1585820269
> > Did you do any manual tests to ensure all the logs are coming through like you expect?
>
> 😬 I will today. I need to tweak a bit the system test to last long enough to have several polls of the logs.
tested live, works well.
--
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