Skip to content

Refactoring utils to be more sane - #1098

Closed
artwr wants to merge 11 commits into
masterfrom
utils_refactor
Closed

Refactoring utils to be more sane#1098
artwr wants to merge 11 commits into
masterfrom
utils_refactor

Conversation

@artwr

@artwr artwr commented Mar 1, 2016

Copy link
Copy Markdown
Contributor

A refactor of the airflow.utils from a file into a folder with topics. As we scale out the Airflow code base, this will help us follow good practices of keeping small modules, and will make finding code easier.

The goal of this PR is to establish foundations to that effect, following the examples set by projects like Django and IPython.

This is more of a starting point, and I welcome comments on how to make this better. I apologize for the sheer size fo the PR, the surgery was extensive.

Note that this will include some backwards incompatible changes in utils. Since utils is mostly used by people extending Airflow, I believe this is still ok, and will help the community grow by making the code base better organized and easier to contribute to.

@mistercrunch

Comment thread airflow/jobs.py Outdated
dag.sla_miss_callback(dag, task_list, blocking_task_list, slas, blocking_tis)
notification_sent = True
from airflow import ascii
from airflow.utils import asciiart

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Scope creep, but can you pull this to the top of the file?

Comment thread airflow/utils/classes.py Outdated
import signal


class AirflowTaskTimeout(Exception):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you meant to remove this from here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was torn on this one since it is used right below in the timeout class. But I will move it to exceptions for consistency.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it is in both places ATM.

@aoen

aoen commented Mar 1, 2016

Copy link
Copy Markdown
Contributor

Thanks for this change, this is great! Looks good, mostly nits.

@artwr

artwr commented Mar 1, 2016

Copy link
Copy Markdown
Contributor Author

That was epic. The build is now passing, let me know if people have more comments!

@aoen

aoen commented Mar 1, 2016

Copy link
Copy Markdown
Contributor

LGTM, thanks for addressing everything!

@mistercrunch

Copy link
Copy Markdown
Member

It's just moving 1k lines around. What could go wrong?!

@aoen

aoen commented Mar 1, 2016

Copy link
Copy Markdown
Contributor

Here's hoping the test coverage is good ^^.

@artwr

artwr commented Mar 1, 2016

Copy link
Copy Markdown
Contributor Author

@mistercrunch The only potential issue that I have seen is the apply_default decorator. It now needs to be imported from airflow.utils.decorators, and so it depends how custom written operators have been written.

It should be a quick fix though.

@artwr

artwr commented Mar 2, 2016

Copy link
Copy Markdown
Contributor Author

@mistercrunch. Bump.

@mistercrunch

Copy link
Copy Markdown
Member

Seems like apply_default should be also namespaced under airflow.utils or it will break all operators in the wild. We do not want to break all the operators that may exist, at least not until a big number version.

@artwr

artwr commented Mar 26, 2016

Copy link
Copy Markdown
Contributor Author

Superseded by #1219
Closing this one.

@artwr artwr closed this Mar 26, 2016
@landscape-bot

Copy link
Copy Markdown

Code Health
Repository health increased by 0.73% when pulling fb86cb2 on utils_refactor into 23cdab0 on master.

@artwr
artwr deleted the utils_refactor branch April 5, 2016 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants