[GitHub] [hive] zabetak commented on a diff in pull request #3603: HIVE-26543: Improve TxnHandler logging

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

zabetak commented on code in PR #3603:
URL: https://github.com/apache/hive/pull/3603#discussion_r976387661


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java:
##########
@@ -716,11 +716,11 @@ private List<Long> openTxns(Connection dbConn, OpenTxnRequest rqst)
 
         if (!targetTxnIdList.isEmpty()) {
           if (targetTxnIdList.size() != rqst.getReplSrcTxnIds().size()) {
-            LOG.warn("target txn id number " + targetTxnIdList.toString() +
-                    " is not matching with source txn id number " + rqst.getReplSrcTxnIds().toString());
+            LOG.warn("target txn id number {} is not matching with source txn id number {}",
+                targetTxnIdList.toString(), rqst.getReplSrcTxnIds().toString());

Review Comment:
   I think `toString` can be removed both for efficiency and code readability. Idem in other places.



##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java:
##########
@@ -945,7 +947,7 @@ private List<Long> getTargetTxnIdList(String replPolicy, List<Long> sourceTxnIdL
       LOG.debug("targetTxnid for srcTxnId " + sourceTxnIdList.toString() + " is " + targetTxnIdList.toString());
       return targetTxnIdList;
     } catch (SQLException e) {
-      LOG.warn("failed to get target txn ids " + e.getMessage());
+      LOG.warn("failed to get target txn ids {}", e.getMessage());

Review Comment:
   The log message along with the whole catch block can be removed. We simply re-throw the exception and the logging the message does not add anything new to the mix; the information that `getTargetTxnIdList` method failed is present in the exception stacktrace.



##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java:
##########
@@ -1004,7 +1006,7 @@ private Set<String> getDbNamesForReplayedTxns(Connection dbConn, List<Long> targ
       }
       return dbNames;
     } catch (SQLException e) {
-      LOG.warn("Failed to get ReplPolicies for replayed txns: " + e.getMessage());
+      LOG.warn("Failed to get ReplPolicies for replayed txns: {}", e.getMessage());
       throw e;
     } finally {

Review Comment:
   Redundant catch block; see previous comment.



##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java:
##########
@@ -2149,8 +2172,8 @@ public AllocateTableWriteIdsResponse allocateTableWriteIds(AllocateTableWriteIds
           if (srcTxnIds.size() != txnIds.size()) {
             // Idempotent case where txn was already closed but gets allocate write id event.
             // So, just ignore it and return empty list.
-            LOG.info("Idempotent case: Target txn id is missing for source txn id : " + srcTxnIds.toString() +
-                    " and repl policy " + rqst.getReplPolicy());
+            LOG.info("Idempotent case: Target txn id is missing for source txn id : {} and repl policy {}",
+                srcTxnIds.toString(), rqst.getReplPolicy());

Review Comment:
   Redundant call `toString()`



##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java:
##########
@@ -1196,11 +1199,13 @@ private void updateDatabaseProp(Connection dbConn, String database,
       }
       closeStmt(pst);
       if (query == null) {
-        LOG.info("Database property: " + prop + " with value: " + propValue + " already updated for db: " + database);
+        LOG.info("Database property: {} with value: {} already updated for db: {}", prop, propValue, database);
         return;
       }
       pst = sqlGenerator.prepareStmtWithParameters(dbConn, query, Arrays.asList(propValue));
-      LOG.debug("Updating " + prop + " for db: " + database + " <" + query.replaceAll("\\?", "{}") + ">", propValue);
+      if (LOG.isDebugEnabled()) {
+        LOG.debug("Updating " + prop + " for db: " + database + " <" + query.replaceAll("\\?", "{}") + ">", propValue);

Review Comment:
   Do we need to use `replaceAll` and pay the price of regex compilation and matching? Wouldn't `replace("?","{}")` suffice? 
   
   Same comment applies to other places where `replaceAll` is used.



-- 
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: gitbox-unsubscribe@hive.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org