Remove error log for pool initialization exception

[JeffFang]

What

One line change to remove error log for pool initialization exception.

Why

For a lib, throwing the exception should be sufficient, and it should be the upstream application's responsibility to determine how to handle the exception, whether to log it out or not. In my particular use case, multiple pools are to be initialized when application starts up, the application tolerates some of pools to be failed, printing exception stacks in error log is misleading in this case.

[codecov]

Codecov Report

Merging #1660 into dev will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                dev    #1660      +/-   ##
============================================
- Coverage     70.11%   70.09%   -0.02%     
  Complexity      561      561              
============================================
  Files            26       26              
  Lines          2118     2117       -1     
  Branches        296      296              
============================================
- Hits           1485     1484       -1     
  Misses          487      487              
  Partials        146      146              

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/com/zaxxer/hikari/pool/HikariPool.java	80.93% <ø> (-0.07%)	59.00 <0.00> (ø)

---

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 c993ef0...ed4f023. Read the comment docs.

[quaff]

You should raise log level to fatal on this logger instead.

[JeffFang]

You should raise log level to fatal on this logger instead.

Thanks for getting back, suppressing logs is apparently an option but it's more like a word-around to me, I raised this more because I think log and throw is a typical anti pattern especially for a downstream library. Printing error log and throwing exception are all ways to notify the failure/error but they're duplicated effort here, simply throwing exception and let upstream application code to decide how to handle should be sufficient, I tried DBCP2 which doesn't have this issue.

I think all log and throws should be eliminated in the lib, however I didn't touch rest of them just want to limit the scope of change.

[quaff]

You should raise log level to fatal on this logger instead.

Thanks for getting back, suppressing logs is apparently an option but it's more like a word-around to me, I raised this more because I think log and throw is a typical anti pattern especially for a downstream library. Printing error log and throwing exception are all ways to notify the failure/error but they're duplicated effort here, simply throwing exception and let upstream application code to decide how to handle should be sufficient, I tried DBCP2 which doesn't have this issue.

I think all log and throws should be eliminated in the lib, however I didn't touch rest of them just want to limit the scope of change.

It doesn't swallow the exception, so it's no anti pattern here, popular lib such as spring do this, I still insist customize your own log level is the best option.

[JeffFang]

You should raise log level to fatal on this logger instead.

Thanks for getting back, suppressing logs is apparently an option but it's more like a word-around to me, I raised this more because I think log and throw is a typical anti pattern especially for a downstream library. Printing error log and throwing exception are all ways to notify the failure/error but they're duplicated effort here, simply throwing exception and let upstream application code to decide how to handle should be sufficient, I tried DBCP2 which doesn't have this issue.
I think all log and throws should be eliminated in the lib, however I didn't touch rest of them just want to limit the scope of change.

It doesn't swallow the exception, so it's no anti pattern here, popular lib such as spring do this, I still insist customize your own log level is the best option.

Thanks for the comment, I never said it does, When I say it's an anti pattern because it does duplicate things to "notify" the failure, which is absolutely fine for an upstream application but not a downstream lib in this case.

[jamey-clari]

Why is there opposition to this? No information is being lost and we avoid clutter in the logs. This is precisely the sort of behavior exception handling is for and the presumption that there needs to be additional logging in this library just adds potentially unwanted clutter. Changing the log level is actually a far more drastic an inappropriate thing to do as it mutes the library from providing possibly valuable debug, info, and warn level information if so configured only to avoid a redundant error log. The behavioral change being requested here is very standard Java exception handling and logging practice. This change is trivial and should be incorporated ASAP.

[jamey-clari]

Happy 2023 all - I'd still love to see this log message suppressed. Simply restricting log level is NOT a solution as it mutes other valuable information. There is no need to log here in addition to throwing (the appropriate action). Please consider incorporating this simple change.

[jamey-clari]

Just revisiting this again, would someone please allow this through. Why is there a belief that the library must log here vs. allowing the caller to make that decision? Changing log level IS NOT a good practice for this scenario as it results in muting other, potentially valuable, information from the library just to avoid this message. Even if other "popular" libs do this (and they do) it isn't good practice. Log and re-through has been widely accepted as undesirable except in very limited circumstances for as long as I've been coding Java (> 25 years).

[lfbayer]

@jamey-clari just to be clear, there has been no opposition to this request. It just hadn't been acted upon.

[jamey-clari]

@lfbayer - thank you for the update, it is truly appreciated. There was some push-back over the past couple of years from another developer on the project and this had languished so my understanding was that was why. I am relieved to see it merged and in the pipeline now as it will help our application logging be less ambiguous as we do "fault-in" DB creation during our normal application behavior. Thanks all!