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-2608] Implements/Standardize custom exceptions for experimental APIs #3496

Closed

Conversation

verdan
Copy link
Contributor

@verdan verdan commented Jun 13, 2018

Make sure you have checked all steps below.

JIRA

  • My PR addresses the following Airflow JIRA issues and references them in the PR title.

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    This PR adds custom exceptions for the APIs, so that we can handle different kind of exceptions efficiently. We can also use these custom exceptions to add the default static message for each exceptions instead of adding that repeatedly in the code. This also helps standardize the status code of each exception.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

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":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff

@codecov-io
Copy link

codecov-io commented Jun 13, 2018

Codecov Report

Merging #3496 into master will increase coverage by 0.02%.
The diff coverage is 89.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3496      +/-   ##
==========================================
+ Coverage    77.4%   77.42%   +0.02%     
==========================================
  Files         204      204              
  Lines       15185    15195      +10     
==========================================
+ Hits        11754    11765      +11     
+ Misses       3431     3430       -1
Impacted Files Coverage Δ
airflow/api/common/experimental/get_task.py 100% <100%> (ø) ⬆️
airflow/api/common/experimental/trigger_dag.py 100% <100%> (ø) ⬆️
airflow/exceptions.py 100% <100%> (ø) ⬆️
...rflow/api/common/experimental/get_dag_run_state.py 100% <100%> (ø) ⬆️
airflow/api/common/experimental/pool.py 100% <100%> (ø) ⬆️
airflow/api/common/experimental/delete_dag.py 82.6% <100%> (-2.58%) ⬇️
airflow/www_rbac/api/experimental/endpoints.py 91.95% <80%> (ø) ⬆️
...rflow/api/common/experimental/get_task_instance.py 90% <80%> (ø) ⬆️
airflow/www/api/experimental/endpoints.py 90.14% <82.6%> (ø) ⬆️
airflow/models.py 88.16% <0%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaa03db...b28e26a. Read the comment docs.

@verdan
Copy link
Contributor Author

verdan commented Jun 14, 2018

@jgao54 @Fokko can I please have some feedback on this PR?

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

This looks good to me, one minor nit here. Bit hesitant with the Exceptions, if we have to create an exception for every possible resource, this can grow quite big. Airflow{Dag,DagRun,Pool}NotFound etc.

status_code = 400


class AirflowNotFoundException(AirflowException):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a docstring here

@verdan verdan force-pushed the AIRFLOW-2608-api-exceptions-handling branch from 8833433 to 028d74a Compare June 15, 2018 11:13
@verdan
Copy link
Contributor Author

verdan commented Jun 15, 2018

That's true, however, I've already added the NotFound exceptions for each of the resources in this PR, and this would help us standardize and differentiate the exceptions based on the objects.

@verdan verdan force-pushed the AIRFLOW-2608-api-exceptions-handling branch 2 times, most recently from 5217b2b to 10eb0fb Compare June 18, 2018 13:36
@verdan verdan force-pushed the AIRFLOW-2608-api-exceptions-handling branch from 10eb0fb to c467b07 Compare June 18, 2018 13:37
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Thanks @verdan for cleaning this up! Cheers

@asfgit asfgit closed this in 5676ec7 Jun 19, 2018
@verdan verdan deleted the AIRFLOW-2608-api-exceptions-handling branch June 19, 2018 08:53
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
…tal APIs

Implements/Standardize custom exceptions for
experimental APIs

Implements/Standardize custom exceptions for
experimental APIs

Closes apache#3496 from verdan/AIRFLOW-2608-api-
exceptions-handling
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.

None yet

3 participants