moj-analytical-services / etl_manager

A python package to create a database on the platform using our moj data warehousing framework
21 stars 9 forks source link

Don't swallow exceptions in GlueJob class #47

Closed xoen closed 6 years ago

xoen commented 6 years ago

Exceptions are a value source of information that can be used to debug problems. By silencing them, the end user (the writer of the Glue Job) will not be able to understand if something went wrong or the job succeeded and will not know what was the cause of the problem: Was it a connectivity issue? was it a credentials problems? was it a quota problem? who knows, it's hard to tell without a stacktrace.

I've added two exception classes:

This is just a starting point, we may want to handle some exceptions and add more exceptions classes but it's a good start I think.

Also, I've removed some of the instanceof checks as type checking is often an antipattern. In this specific case we were expecting a dictionary but if that's not the case it means something went wrong before, so there should be an exception (unless we know how to handle it properly, and pass is just silencing a problem).

NOTE: This may potentially break the client code.

codecov-io commented 6 years ago

Codecov Report

Merging #47 into master will increase coverage by 1.5%. The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #47     +/-   ##
=========================================
+ Coverage   72.55%   74.06%   +1.5%     
=========================================
  Files           4        4             
  Lines         809      798     -11     
=========================================
+ Hits          587      591      +4     
+ Misses        222      207     -15
Impacted Files Coverage Δ
etl_manager/etl.py 58.01% <33.33%> (+4.65%) :arrow_up:

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 7ce7cdd...7d9d8f7. Read the comment docs.

xoen commented 6 years ago

Let's double check with out Python ninja @r4vi if this makes sense to him :)

isichei commented 6 years ago

fixes #45