Skip to content

Conversation

@ephraimbuddy
Copy link
Contributor

@ephraimbuddy ephraimbuddy commented Jun 18, 2020


Currently the logic for reading task logs is implemented inside views.py.

This logic is also needed in API and it would be better if the logic is extracted from the view and tested separately. This will enable reuse of the logic in API log_endpoint.

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@boring-cyborg boring-cyborg bot added area:logging area:webserver Webserver related Issues labels Jun 18, 2020
@ephraimbuddy ephraimbuddy mentioned this pull request Jun 18, 2020
6 tasks
Copy link
Member

Choose a reason for hiding this comment

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

Does Javascript need to be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything is fine with the logging on the website

@mik-laj mik-laj changed the title Extract TaskLogReader from views for uniform use in API & webserver and improve tests Extract TaskLogReader from views Jun 18, 2020
@mik-laj mik-laj changed the title Extract TaskLogReader from views Extract TaskLogReader from views.py Jun 18, 2020
@mik-laj
Copy link
Member

mik-laj commented Jun 18, 2020

@KevinYang21 Could you look at it? We need to extract logic to be able to reuse it in the API.@KevinYang21

Copy link
Member

Choose a reason for hiding this comment

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

Can you add return type?

Copy link
Member

Choose a reason for hiding this comment

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

Can you add return type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed f829810

@mik-laj mik-laj added the area:API Airflow's REST/HTTP API label Jun 18, 2020
Copy link
Member

Choose a reason for hiding this comment

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

Could you describe here how to use this function? I would be happy to add a code snippet to explain how to use metadata.

            while 'end_of_log' not in metadata or not metadata['end_of_log']:
                logs, metadata = self.read_log_chunks(ti, current_try_number, metadata)
               for log in logs:
                   print(log)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check this 8c383bf

@kaxil
Copy link
Member

kaxil commented Jun 18, 2020

Please add a description to the PR

@kaxil
Copy link
Member

kaxil commented Jun 18, 2020

If we checkmark the box, let's actually follow the guidelines:

image

@ephraimbuddy
Copy link
Contributor Author

Hi @kaxil, please take a look, I have updated the description. Thanks for pointing this out.

@mik-laj
Copy link
Member

mik-laj commented Jun 19, 2020

@ephraimbuddy Can you do a rebase?

@mik-laj mik-laj merged commit eb8683a into apache:master Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:API Airflow's REST/HTTP API area:logging area:webserver Webserver related Issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants