[GitHub] [zookeeper] belugabehr commented on a change in pull request #1084: ZOOKEEPER-3541: Wrong placeholder '{}' in logs.

[GitBox <gi...@apache.org>]

belugabehr commented on a change in pull request #1084: ZOOKEEPER-3541: Wrong placeholder '{}' in logs.
URL: https://github.com/apache/zookeeper/pull/1084#discussion_r322761573
 
 

 ##########
 File path: zookeeper-server/src/main/java/org/apache/zookeeper/client/ZooKeeperSaslClient.java
 ##########
 @@ -274,7 +274,7 @@ public void respondToServer(byte[] serverToken, ClientCnxn cnxn) {
             } catch (SaslException e) {
                 LOG.error("SASL authentication failed using login context '"
                           + this.getLoginContext()
-                          + "' with exception: {}", e);
 
 Review comment:
   Ya, this example is a bit odd.
   
   ```
                   LOG.error("SASL authentication failed using login context '"
                             + this.getLoginContext()
                             + "' with exception: {}", e);
   ```
   
   It is using parameters and concatenation.  Typically one or the other is used, not both.
   
   `Throwable` objects are handled special.  They do not need the marker `{}` to be included in the message, they just need to be the last argument in the list.  If a stack trace is not desirable, then  using a marker in conjunction with `e.getMessage()` can be used.
   
   Something better would be:
   ```
   LOG.error("SASL authentication failed using login context '{}'", this.getLoginContext(), e);
   ```
   
   The login context is passed to the marker, and the `Exception` message (plus stack trace) is printed as well.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services