Don't log & throw in Database.execute* methods. Fixes #81

[codecov-io]

Codecov Report

Merging #82 into master will increase coverage by 0.16%. The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
+ Coverage    81.7%   81.87%   +0.16%     
==========================================
  Files          43       43              
  Lines        1148     1142       -6     
  Branches      102      103       +1     
==========================================
- Hits          938      935       -3     
+ Misses        210      207       -3

Impacted Files	Coverage Δ
...lest/src/main/scala/sqlest/executor/Database.scala	66.17% <85.71%> (+0.68%)	: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 3804ac7...c9bc818. Read the comment docs.

[kutchar]

@DavidGregory084 Any feedback/update on this?

[DavidGregory084]

Sorry @kutchar - I missed this somehow! I will take a look at this PR tomorrow.

[kutchar]

No worries. Thanks.

[DavidGregory084]

Hi @kutchar, I understand your point about log & throw.

Perhaps this is a peculiarity in the way that we approach logging at JHC, but we do relatively little logging at the application level. If we can't handle an error in-process, we don't tend to catch the error: the process just dies. We make very little use of exceptions in our application logic so exceptions are pretty much considered fatal. That makes logging at library level particularly important.

I think that removing this logging will be problematic for us as it removes information that our operations staff need to diagnose environmental issues that come up from time to time (journalling disabled, SQL functions missing). OTOH, we do have a few additional layers of code above sqlest in which I could do this.

@p14n, what do you think?

[kutchar]

@DavidGregory084 I see your point and it's nice to have sqlest log those exceptions with more context (i.e. the query) which are not available at layers above.

Another option would be to introduce a way for users to decide what exceptions should be logged, i.e. adding def shouldLogException(e: Throwable)in Database that by default would log everything. We could accomplish that by either adding a method to Database that can be overriden or adding two more Database.withDataSource methods that also take filter functions.

I like this approach since it wouldn't change the behavior of the library too much since others might be depending on these exception logs also.

[p14n]

"I think that removing this logging will be problematic for us as it removes information that our operations staff need to diagnose environmental issues that come up from time to time (journalling disabled, SQL functions missing). OTOH, we do have a few additional layers of code above sqlest in which I could do this."

I agree with @kutchar on this. I'm happy with either removing the logging or adding shouldThrow. I think I have a preference for removing the logging. If we do this, we should make clear in the release notes for the new version. If we remove the logging, we should scan as many of our internal test instances for errors from this class and check that for that request the error has been logged further up the stack.

[kutchar]

Hey guys, any update on this?

[DavidGregory084]

@kutchar Sorry for keeping you waiting! Been struggling to set aside time to work on the changes we need to make on our end but that shouldn't block your PR.

[kutchar]

@DavidGregory084 No problem, thanks for merging it. Any idea when you'd be able to release it?

[DavidGregory084]

Should have been on its way but I'm wrangling with sbt-pgp - bear with me

[DavidGregory084]

Okay, 0.8.10 should be on its way to central now, thanks @kutchar!

[kutchar]

Thanks!