Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-5027] Get logs from CloudWatch for ECSOperators #5645

Merged
merged 1 commit into from
Aug 22, 2019
Merged

[AIRFLOW-5027] Get logs from CloudWatch for ECSOperators #5645

merged 1 commit into from
Aug 22, 2019

Conversation

StevenReitsma
Copy link
Contributor

My first Airflow PR so I hope I adhered to all the contribution guidelines. All comments welcome.


Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:

It's very useful to have the CloudWatch task logs of an ECS/SageMaker job available within Airflow. Currently, this is not possible for ECS tasks.

The SageMaker operator starts a job on AWS SageMaker. It already supports grabbing the CloudWatch logs of the (finished) job to the Airflow instance.

The ECS operator is very similar in that it starts a job on AWS ECS. It doesn't, however, support grabbing CloudWatch logs at the end of a job.

I've separated the existing log grabbing of the SageMaker hook to a separate logs hook and both the SageMaker hook and ECS operator now call that logs hook at the end of a job.

Tests

  • My PR adds the following unit tests:

Added: tests.contrib.hooks.TestAwsLogsHook
Changed: tests.contrib.hooks.TestSageMakerHook
Changed: tests.contrib.sensors.TestSageMakerTrainingSensor

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does

Code Quality

  • Passes flake8

@potiuk
Copy link
Member

potiuk commented Jul 23, 2019

@StevenReitsma please rebase and push so that Travis can build it. Travis had a problem whole day yesterday.

@mik-laj mik-laj added the provider:amazon-aws AWS/Amazon - related issues label Jul 27, 2019
@StevenReitsma
Copy link
Contributor Author

Could someone take a look at this PR?

@ashb ashb changed the title [AIRFLOW-5027] Generalized CloudWatch log grabbing for ECS and SageMaker operators [AIRFLOW-5027] Get logs from CloudWatch for ECSOperators Aug 13, 2019
@StevenReitsma
Copy link
Contributor Author

PTAL @ashb

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more small changes please.

airflow/contrib/hooks/sagemaker_hook.py Outdated Show resolved Hide resolved
airflow/contrib/hooks/sagemaker_hook.py Outdated Show resolved Hide resolved
airflow/contrib/hooks/sagemaker_hook.py Outdated Show resolved Hide resolved
airflow/contrib/operators/ecs_operator.py Outdated Show resolved Hide resolved
@StevenReitsma
Copy link
Contributor Author

Thanks for the reviews so far @ashb, PTAL :)

@StevenReitsma
Copy link
Contributor Author

@ashb Made the change and Travis is green again. PTAL :)

@ashb ashb merged commit 786db8b into apache:master Aug 22, 2019
ashb pushed a commit to ashb/airflow that referenced this pull request Aug 22, 2019
kaxil pushed a commit that referenced this pull request Aug 30, 2019
Jerryguo pushed a commit to Jerryguo/airflow that referenced this pull request Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants