Skip to content

Conversation

@lilgreenbird
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Nov 16, 2018

Codecov Report

Merging #883 into dev will increase coverage by 1.59%.
The diff coverage is 32.54%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #883      +/-   ##
============================================
+ Coverage     48.58%   50.18%   +1.59%     
- Complexity     2803     2813      +10     
============================================
  Files           118      118              
  Lines         27868    27022     -846     
  Branches       4640     4241     -399     
============================================
+ Hits          13541    13562      +21     
+ Misses        12131    11512     -619     
+ Partials       2196     1948     -248
Flag Coverage Δ Complexity Δ
#JDBC42 49.69% <32.41%> (+1.56%) 2770 <118> (+9) ⬆️
#JDBC43 50.07% <32.54%> (+1.51%) 2804 <122> (+2) ⬆️
Impacted Files Coverage Δ Complexity Δ
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java 35.53% <ø> (+3.1%) 256 <0> (+3) ⬆️
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java 44.44% <ø> (+0.24%) 109 <0> (+3) ⬆️
...om/microsoft/sqlserver/jdbc/AuthenticationJNI.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../microsoft/sqlserver/jdbc/SQLServerXAResource.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...microsoft/sqlserver/jdbc/FailOverMapSingleton.java 14.28% <0%> (+0.49%) 0 <0> (ø) ⬇️
...m/microsoft/sqlserver/jdbc/KerbAuthentication.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...icrosoft/sqlserver/jdbc/SQLServerXAConnection.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...microsoft/sqlserver/jdbc/SQLServerADAL4JUtils.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...icrosoft/sqlserver/jdbc/SQLServerXADataSource.java 8.33% <0%> (+2.08%) 2 <0> (ø) ⬇️
.../com/microsoft/sqlserver/jdbc/SQLServerDriver.java 80.59% <0%> (+0.36%) 25 <0> (ø) ⬇️
... and 33 more

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 1fe2515...e528538. Read the comment docs.

@cheenamalhotra cheenamalhotra added this to the 7.1.3 milestone Nov 20, 2018
@ulvii
Copy link
Contributor

ulvii commented Nov 21, 2018

Please remove unused imports from all files, I noticed some files still contain unnecessary import java.util.logging.Level;.

rene-ye
rene-ye previously approved these changes Nov 23, 2018
cheenamalhotra
cheenamalhotra previously approved these changes Nov 28, 2018
@lilgreenbird lilgreenbird merged commit 25028fb into microsoft:dev Nov 28, 2018
Copy link

@jardelnovaes jardelnovaes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a good idea!
You are creating a new String by the concatenation of the other strings even if the log is not FINE!
and you are executing the unnecessary logic of toString() as well.

When you are using a log framework that allows wildcard like this

log.debug("My message is {} and don't create unecessary string. Message 2 {}", myStringA, myStringB, ...)

You won't create unnecessary string, but if you use methods you will execute them even when the loglevel is not that one and that is not a good idea. When you have method is better to use previous code. If you concatenate string as well.

There are case that NOW will execute string concatenations, getClass(), exception.getMessage() unnecessarily.

A JDBC Driver Should be as fast as possible and avoid every unnecessary resource!

I got two references of the same survey that is based on Git repositories:
http://blog.logscape.com/2017/03/concatenation-or-parameters-both-whats-the-top-method-of-java-logging/
https://blog.takipi.com/whats-the-top-java-logging-method-on-github-string-concatenation-vs-parameterized-logging/

rene-ye added a commit to rene-ye/mssql-jdbc that referenced this pull request Nov 29, 2018
@lilgreenbird
Copy link
Contributor Author

hi @jardelnovaes
Thank you, you are correct. We are now reverting this PR and will look at the options you've suggested. Thanks for the catch!

lilgreenbird pushed a commit that referenced this pull request Nov 29, 2018
ulvii pushed a commit that referenced this pull request Nov 30, 2018
* removed redundant check of logger level

* removed redundant check of logger level

* removed unused imports

* resolved conflicts
ulvii pushed a commit that referenced this pull request Nov 30, 2018
ulvii added a commit that referenced this pull request Nov 30, 2018
ulvii added a commit that referenced this pull request Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants