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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
