Fix for static logger member in abstract class 'SQLServerClobBase'

[cheenamalhotra]

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.

[codecov-io]

Codecov Report

Merging #537 into dev will decrease coverage by 0.05%.
The diff coverage is 83.33%.

@@             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

Flag	Coverage Δ	Complexity Δ
#JDBC41	46.38% <83.33%> (-0.06%)	2206 <4> (-7)
#JDBC42	46.52% <83.33%> (-0.06%)	2213 <4> (-11)

Impacted Files	Coverage Δ	Complexity Δ
...a/com/microsoft/sqlserver/jdbc/SQLServerNClob.java	100% <100%> (ø)	3 <2> (-1)	⬇️
...va/com/microsoft/sqlserver/jdbc/SQLServerClob.java	27.94% <75%> (-0.32%)	3 <2> (-1)
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java	44.94% <0%> (-2.25%)	16% <0%> (ø)
...om/microsoft/sqlserver/jdbc/SimpleInputStream.java	51.11% <0%> (-1.49%)	11% <0%> (-1%)
...va/com/microsoft/sqlserver/jdbc/StreamColumns.java	87.83% <0%> (-1.36%)	21% <0%> (-1%)
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java	32.37% <0%> (-0.78%)	238% <0%> (-7%)
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java	59.36% <0%> (-0.11%)	130% <0%> (-1%)
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java	52.58% <0%> (+0.06%)	239% <0%> (-1%)	⬇️
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java	54.12% <0%> (+0.26%)	0% <0%> (ø)	⬇️
...n/java/com/microsoft/sqlserver/jdbc/Parameter.java	62.44% <0%> (+0.42%)	63% <0%> (ø)	⬇️

---

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 5c75147...93e0b18. Read the comment docs.

[ulvii]

How about this comment that got deleted?
// Loggers should be class static to avoid lock contention with multiple threads

[cheenamalhotra]

Well - its because this logger object won't be static in this case.

[peterbae]

test result looks good.

[marschall]

There is now a logger lookup whenever an instance of SQLServerClob or SQLServerNClob is created. This could be avoided if:

1. there is a static final logger in SQLServerClob or SQLServerNClob, like before
2. the logger in SQLServerClobBase is made private final, like in this PR
3. SQLServerClob and SQLServerNClob pass the logger in the super constructor call

[cheenamalhotra]

Hi @marschall

Thanks for replying back! We could go for either solution, but I removed the loggers from sub classes as:

1. Class Lookup isn't expensive here.
2. The loggers in sub classes are not used anywhere else except being passed in constructor (but remain attached in memory with class objects)

[marschall]

It's not the class lookup that I was concerned about, it's get logger lookup.

[cheenamalhotra]

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.

[marschall]

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:

1. Reflection.getCallerClass() which creates an exception and walks the entire stack
2. check for a security manager several times
3. 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.

[cheenamalhotra]

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.