This is an automated email from the ASF dual-hosted git repository. stigahuang pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit d09940b5ddc0dfcbbfe6e8d23f4153469d86d12d Author: Michael Smith <[email protected]> AuthorDate: Thu Oct 10 15:38:20 2024 -0700 IMPALA-13563: Cleanup logging Cleans up calls to logDebug and a few other locations: - exit early if producing debug message input is expensive - use slf4j parameterized logging - normalize on logDebug handling isDebugEnabled checks Change-Id: I32e1c62511c292d36aa879c60ae3d91ed4f65697 Reviewed-on: http://gerrit.cloudera.org:8080/22090 Reviewed-by: Michael Smith <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> --- fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java | 4 ++-- fe/src/main/java/org/apache/impala/analysis/SlotRef.java | 3 ++- fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java | 8 ++++---- .../impala/catalog/metastore/CatalogMetastoreServiceHandler.java | 4 ++-- .../org/apache/impala/calcite/service/CalciteJniFrontend.java | 4 +--- .../org/apache/impala/calcite/service/CalciteMetadataHandler.java | 7 ++++--- .../org/apache/impala/calcite/service/CalcitePhysPlanCreator.java | 4 ++-- .../org/apache/impala/calcite/service/CalciteQueryParser.java | 4 ++-- .../java/org/apache/impala/calcite/service/CalciteValidator.java | 4 ++-- .../org/apache/impala/calcite/service/ExecRequestCreator.java | 4 ++-- 10 files changed, 23 insertions(+), 23 deletions(-) diff --git a/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java b/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java index 0a6b7161c..ff04ff7cc 100644 --- a/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java +++ b/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java @@ -261,8 +261,8 @@ public class ArithmeticExpr extends Expr { fn_ = getBuiltinFunction(analyzer, fnName, collectChildReturnTypes(), CompareMode.IS_IDENTICAL); if (fn_ == null) { - Preconditions.checkState(false, String.format("No match " + - "for '%s' with operand types %s and %s", toSql(), t0, t1)); + Preconditions.checkState(false, + "No match for '%s' with operand types %s and %s", toSql(), t0, t1); } Preconditions.checkState(type_.matchesType(fn_.getReturnType())); } diff --git a/fe/src/main/java/org/apache/impala/analysis/SlotRef.java b/fe/src/main/java/org/apache/impala/analysis/SlotRef.java index f1ede3235..86378b452 100644 --- a/fe/src/main/java/org/apache/impala/analysis/SlotRef.java +++ b/fe/src/main/java/org/apache/impala/analysis/SlotRef.java @@ -330,7 +330,8 @@ public class SlotRef extends Expr { // we shouldn't be sending exprs over non-materialized slots Preconditions.checkState(desc_.isMaterialized(), "Illegal reference to non-materialized slot: %s", this); - Preconditions.checkState(desc_.getByteOffset() >= 0); + Preconditions.checkState(desc_.getByteOffset() >= 0, + "computeMemLayout not done for slot: %s", this); // check that the tuples associated with this slot are executable desc_.getParent().checkIsExecutable(); if (desc_.getItemTupleDesc() != null) desc_.getItemTupleDesc().checkIsExecutable(); diff --git a/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java b/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java index 41fe67452..bbc73964d 100644 --- a/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java +++ b/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java @@ -292,12 +292,12 @@ public class TupleDescriptor { * is not executable, i.e., if one of those conditions is not met. */ public void checkIsExecutable() { - Preconditions.checkState(isMaterialized_, String.format( + Preconditions.checkState(isMaterialized_, "Illegal reference to non-materialized tuple: debugname=%s alias=%s tid=%s", - debugName_, StringUtils.defaultIfEmpty(getAlias(), "n/a"), id_)); - Preconditions.checkState(hasMemLayout_, String.format( + debugName_, StringUtils.defaultIfEmpty(getAlias(), "n/a"), id_); + Preconditions.checkState(hasMemLayout_, "Missing memory layout for tuple: debugname=%s alias=%s tid=%s", - debugName_, StringUtils.defaultIfEmpty(getAlias(), "n/a"), id_)); + debugName_, StringUtils.defaultIfEmpty(getAlias(), "n/a"), id_); } /** diff --git a/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java b/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java index dcdd0f551..75a326100 100644 --- a/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java +++ b/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java @@ -1331,10 +1331,10 @@ public class CatalogMetastoreServiceHandler extends MetastoreServiceHandler { MetastoreEventsProcessor.getNextMetastoreEventsInBatchesForTable(catalog_, currentEventId, newTable.getDbName(), newTable.getTableName(), MetastoreEvents.AlterTableEvent.EVENT_TYPE); - Preconditions.checkState(events.size() == 1, String.format("For table %s.%s, " + Preconditions.checkState(events.size() == 1, "For table %s.%s, " + "from event id: %s, expected ALTER_TABLE events size to be 1 but is %s", newTable.getDbName(), newTable.getTableName(), currentEventId, - events.size())); + events.size()); MetastoreEvents.MetastoreEvent event = metastoreEventFactory_.get(events.get(0), metastoreEventsMetrics_); diff --git a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java index 152c36d00..bcba489c0 100644 --- a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java +++ b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java @@ -199,9 +199,7 @@ public class CalciteJniFrontend extends JniFrontend { QueryContext queryCtx, String stepMessage) { LOG.info(stepMessage); queryCtx.getTimeline().markEvent(stepMessage); - if (LOG.isDebugEnabled()) { - compilerStep.logDebug(stepResult); - } + compilerStep.logDebug(stepResult); } private static void loadCalciteImpalaFunctions() { diff --git a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java index 3634be748..67e4e5674 100644 --- a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java +++ b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java @@ -354,9 +354,10 @@ public class CalciteMetadataHandler implements CompilerStep { @Override public void logDebug(Object resultObject) { - LOG.debug("Loaded tables: " + stmtTableCache_.tables.values().stream() + if (!LOG.isDebugEnabled()) return; + String allTables = stmtTableCache_.tables.values().stream() .map(feTable -> feTable.getName().toString()) - .collect(Collectors.joining( ", " ))); + .collect(Collectors.joining( ", " )); + LOG.debug("Loaded tables: {}", allTables); } } - diff --git a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalcitePhysPlanCreator.java b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalcitePhysPlanCreator.java index 551cdcd3f..a1950345b 100644 --- a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalcitePhysPlanCreator.java +++ b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalcitePhysPlanCreator.java @@ -88,9 +88,9 @@ public class CalcitePhysPlanCreator implements CompilerStep { @Override public void logDebug(Object resultObject) { if (!(resultObject instanceof NodeWithExprs)) { - LOG.debug("Finished physical plan step, but unknown result: " + resultObject); + LOG.debug("Finished physical plan step, but unknown result: {}", resultObject); return; } - LOG.debug("Physical Plan: " + resultObject); + LOG.debug("Physical Plan: {}", resultObject); } } diff --git a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteQueryParser.java b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteQueryParser.java index 90ba2e6af..7edf6def1 100644 --- a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteQueryParser.java +++ b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteQueryParser.java @@ -65,9 +65,9 @@ public class CalciteQueryParser implements CompilerStep { public void logDebug(Object resultObject) { if (!(resultObject instanceof SqlNode)) { - LOG.debug("Parser produced an unknown output: " + resultObject); + LOG.debug("Parser produced an unknown output: {}", resultObject); return; } - LOG.debug("Parsed node: " + resultObject); + LOG.debug("Parsed node: {}", resultObject); } } diff --git a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java index 43fd63913..11d2d6d6a 100644 --- a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java +++ b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java @@ -93,9 +93,9 @@ public class CalciteValidator implements CompilerStep { @Override public void logDebug(Object resultObject) { if (!(resultObject instanceof SqlNode)) { - LOG.debug("Finished validator step, but unknown result: " + resultObject); + LOG.debug("Finished validator step, but unknown result: {}", resultObject); return; } - LOG.debug("Validated node: " + resultObject); + LOG.debug("Validated node: {}", resultObject); } } diff --git a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java index b0dd1ec29..3e5b90913 100644 --- a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java +++ b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java @@ -427,8 +427,8 @@ public class ExecRequestCreator implements CompilerStep { @Override public void logDebug(Object resultObject) { if (!(resultObject instanceof TExecRequest)) { - LOG.debug("Finished create exec request step, but unknown result: " + resultObject); + LOG.debug("Finished exec request step, but unknown result: {}", resultObject); } - LOG.debug("Exec request: " + resultObject); + LOG.debug("Exec request: {}", resultObject); } }
