[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #7375: GEODE-6588: Fixed mismatch of placeholders and arguments

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

pivotal-jbarrett commented on a change in pull request #7375:
URL: https://github.com/apache/geode/pull/7375#discussion_r808510116



##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueStateImpl.java
##########
@@ -310,7 +310,7 @@ void sendPeriodicAck() {
           success = true;
         } catch (Exception ex) {
           if (logger.isDebugEnabled()) {
-            logger.debug("Exception while sending an ack to the primary server: {}", ex);
+            logger.debug("Exception while sending an ack to the primary server:", ex);

Review comment:
       Is it possible that this production code intends to just log the message and not the entire stack trace?

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java
##########
@@ -328,8 +328,8 @@ protected void basicPerformTimeout(boolean isPending) throws CacheException {
           if (logger.isTraceEnabled()) {
             // NOTE: original finer message used this.toString() twice

Review comment:
       I don't this comment is necessary anymore.

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/locks/TXLessorDepartureHandler.java
##########
@@ -108,8 +108,8 @@ private void sendRecoveryMsgs(final DistributionManager dm, final DLockBatch[] b
       // this shouldn't happen unless we're shutting down or someone has set a size constraint
       // on the waiting-pool using a system property
       if (!dm.getCancelCriterion().isCancelInProgress()) {
-        logger.warn("Unable to schedule background cleanup of transactions for departed member {}."
-            + "  Performing in-line cleanup of the transactions.");
+        logger.warn(
+            "Unable to schedule background cleanup of transactions for departed member. Performing in-line cleanup of the transactions.");

Review comment:
       Is there a member to log here?

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/execute/ServerToClientFunctionResultSender.java
##########
@@ -98,7 +98,7 @@ public synchronized void lastResult(Object oneResult) {
     }
 
     if (logger.isDebugEnabled()) {
-      logger.debug("ServerToClientFunctionResultSender sending last result1 {} " + oneResult);
+      logger.debug("ServerToClientFunctionResultSender sending last result1 {} ", oneResult);

Review comment:
       Trailing whitespace.

##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/jta/dunit/LoginTimeOutDUnitTest.java
##########
@@ -263,15 +263,15 @@ public static void runTest1() throws Exception {
         logger.debug("Naming Exception caught in lookup: " + e);
         fail("failed in naming lookup: " + e);
       } catch (Exception e) {
-        logger.debug("Exception caught during naming lookup: {}", e);
+        logger.debug("Exception caught during naming lookup:", e);
         fail("failed in naming lookup: " + e);
       }
       try {
         for (int count = 0; count < MAX_CONNECTIONS; count++) {
           ds.getConnection();
         }
       } catch (Exception e) {
-        logger.debug("Exception caught in runTest1: {}", e);
+        logger.debug("Exception caught in runTest1:", e);

Review comment:
       Is it possible that these log messages intended to log the message only and not the stacktrace?

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionDataStore.java
##########
@@ -1257,7 +1257,7 @@ boolean canAccommodateMoreBytesSafely(int bytes) {
     final long curBytes = bytesInUse.get();
     if (isDebugEnabled) {
       logger.debug(
-          "canAccomodateMoreBytes: bytes = {} allocatedMemory = {} newAllocatedSize = {} thresholdSize = ",
+          "canAccomodateMoreBytes: bytes ={} allocatedMemory ={} newAllocatedSize ={} thresholdSize ={}",

Review comment:
       Was it intentional to remove the whitespace here?

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/UpdateAttributesProcessor.java
##########
@@ -462,7 +462,7 @@ public static void send(InternalDistributedMember recipient, int processorId,
       if (exception != null) {
         m.setException(exception);
         if (logger.isDebugEnabled()) {
-          logger.debug("Replying with exception: {}" + m, exception);
+          logger.debug("Replying with exception:{}", m, exception);

Review comment:
       I wonder if the intent was for just the exception message here and not the stack.

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionDataStore.java
##########
@@ -3022,7 +3022,7 @@ public void handleInterestEvent(InterestRegistrationEvent event) {
         // If this is a registration event, add interest for this key
         if (isRegister) {
           if (logger.isDebugEnabled()) {
-            logger.debug("PartitionedRegionDataStore for {} adding interest for: ",
+            logger.debug("PartitionedRegionDataStore for {} adding interest for :{}",

Review comment:
       Whitespace change seems wrong.




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org