cache JDBC dbInfo after connection constructor throws exception#322
Conversation
realark
changed the title
gary-huang
added
type: bug
tylerbenson
left a comment
tylerbenson
left a comment
There was a problem hiding this comment.
Good solution to the problem. It's too bad we couldn't add the exception handling to the constructor instrumentation. You're going to need to rebase off master and resolve the conflicts.
I suggested a few minor changes while you're working on this.
There was a problem hiding this comment.
Here's something you could consider using if you want to extend the testing to mysql/postgres: https://www.testcontainers.org/
There was a problem hiding this comment.
Thanks for the comment @tylerbenson, @realark and I discussed it and thought that it would be better to include test containers when there are more test cases, since we do need Docker to perform unit tests using test containers, and that introduces another development dependency. Is this OK?
There was a problem hiding this comment.
We should consider replacing our synchronized weak maps with WeakConcurrentMap. I'm guessing they are much more performant than our current solution.
gary-huang
force-pushed
the
gary/hikari-handling
branch
2 times, most recently
from
865e97f to
2a0260d
Compare
Client will now report correct database if JDBC connection is recovered from exception.
Refactored JDBCMaps's getDBInfo utlity function because JDBCMaps is in the bootstrap classloader, and the use of java.sql.* packages in getDBInfo is failing because java.sql.* packages are part of the platform classloader in java 9.
gary-huang
force-pushed
the
gary/hikari-handling
branch
from
17f619c to
a7115a5
Compare
gary-huang
force-pushed
the
gary/hikari-handling
branch
from
a7115a5 to
71396ae
Compare
3 participants
Fixed the "unknown" database issue caused by JDBC Connection constructor throwing exception. Tracer will now attempt to record database info on JDBC statement creation or preparation because connection would've been properly established.