Fix for static logger member in abstract class 'SQLServerClobBase'#537
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #537 +/- ##
============================================
- Coverage 46.68% 46.62% -0.06%
+ Complexity 2228 2215 -13
============================================
Files 108 108
Lines 25308 25306 -2
Branches 4175 4175
============================================
- Hits 11815 11799 -16
- Misses 11463 11476 +13
- Partials 2030 2031 +1
Continue to review full report at Codecov.
|
| transient SQLServerConnection con; | ||
| private static Logger logger; | ||
|
|
||
| private final Logger logger = Logger.getLogger(getClass().getName()); |
There was a problem hiding this comment.
How about this comment that got deleted?
// Loggers should be class static to avoid lock contention with multiple threads
There was a problem hiding this comment.
Well - its because this logger object won't be static in this case.
peterbae
left a comment
There was a problem hiding this comment.
test result looks good.
| transient SQLServerConnection con; | ||
| private static Logger logger; | ||
|
|
||
| private final Logger logger = Logger.getLogger(getClass().getName()); |
There was a problem hiding this comment.
There is now a logger lookup whenever an instance of SQLServerClob or SQLServerNClob is created. This could be avoided if:
- there is a
static finallogger inSQLServerCloborSQLServerNClob, like before - the logger in
SQLServerClobBaseis madeprivate final, like in this PR SQLServerClobandSQLServerNClobpass the logger in thesuperconstructor call
There was a problem hiding this comment.
Hi @marschall
Thanks for replying back! We could go for either solution, but I removed the loggers from sub classes as:
- Class Lookup isn't expensive here.
- The loggers in sub classes are not used anywhere else except being passed in constructor (but remain attached in memory with class objects)
There was a problem hiding this comment.
It's not the class lookup that I was concerned about, it's get logger lookup.
There was a problem hiding this comment.
Can you please explain what you mean by Logger lookup and how will it be different in the other case - maybe I am not able to understand your point.
There was a problem hiding this comment.
With a non-static logger in SQLServerClob or SQLServerNClob every time a new instance of these classes is generated the following is executed:
Logger.getLogger(getClass().getName())If we ignore getClass().getName() for now that still leaves us with Logger.getLogger() which does among other things:
Reflection.getCallerClass()which creates an exception and walks the entire stack- check for a security manager several times
- goes through a global lock in
java.util.logging.LogManager.LoggerContext#namedLoggers
If you make the loggers in SQLServerClob and SQLServerNClob static all that work happens only one during class loading.
There was a problem hiding this comment.
Hi @marschall I understand the concern for non-static logger here. Made changes in the new PR #563 as discussed above. Request you to review the PR so we can get it working.
Fixes issue #186.
This change makes logger in abstract class 'SQLServerClobBase' non-static but final such that it is instantiated for every new object, based on subclass been used to create object and is immutable.