fix: Fix trace exception and code refactor (facebookincubator#11965)

Summary: 1. "VELOX_SPILL_LIMIT_EXCEEDED" -> "VELOX_TRACE_LIMIT_EXCEEDED" in updateTracedBytesAndCheckLimit(). 2. Typo in the comment of OperatorTraceTest. Pull Request resolved: facebookincubator#11965 Reviewed By: gggrace14 Differential Revision: D67653883 Pulled By: xiaoxmeng fbshipit-source-id: 4effe4637a2ccc8c103721087b854059b60c53a2
bikramSingh91 · Dec 27, 2024 · c046524 · c046524
1 parent 646d4a9
commit c046524
Show file tree

Hide file tree

Showing 10 changed files with 18 additions and 17 deletions.
diff --git a/velox/common/base/CMakeLists.txt b/velox/common/base/CMakeLists.txt
@@ -38,7 +38,8 @@ velox_add_library(
   SpillConfig.cpp
   SpillStats.cpp
   StatsReporter.cpp
-  SuccinctPrinter.cpp)
+  SuccinctPrinter.cpp
+  TraceConfig.cpp)
 
 velox_link_libraries(
   velox_common_base

diff --git a/velox/exec/TraceConfig.cpp → velox/common/base/TraceConfig.cpp b/velox/exec/TraceConfig.cpp → velox/common/base/TraceConfig.cpp
@@ -14,11 +14,10 @@
  * limitations under the License.
  */
 
-#include "velox/exec/TraceConfig.h"
-
 #include <utility>
 
 #include "velox/common/base/Exceptions.h"
+#include "velox/common/base/TraceConfig.h"
 
 namespace facebook::velox::exec::trace {
 

diff --git a/velox/exec/TraceConfig.h → velox/common/base/TraceConfig.h b/velox/exec/TraceConfig.h → velox/common/base/TraceConfig.h
@@ -23,6 +23,15 @@
 
 namespace facebook::velox::exec::trace {
 
+#define VELOX_TRACE_LIMIT_EXCEEDED(errorMessage)                    \
+  _VELOX_THROW(                                                     \
+      ::facebook::velox::VeloxRuntimeError,                         \
+      ::facebook::velox::error_source::kErrorSourceRuntime.c_str(), \
+      ::facebook::velox::error_code::kTraceLimitExceeded.c_str(),   \
+      /* isRetriable */ true,                                       \
+      "{}",                                                         \
+      errorMessage);
+
 /// The callback used to update and aggregate the trace bytes of a query. If the
 /// query trace limit is set, the callback return true if the aggregate traced
 /// bytes exceed the set limit otherwise return false.

diff --git a/velox/core/QueryCtx.cpp b/velox/core/QueryCtx.cpp
@@ -16,6 +16,7 @@
 
 #include "velox/core/QueryCtx.h"
 #include "velox/common/base/SpillConfig.h"
+#include "velox/common/base/TraceConfig.h"
 #include "velox/common/config/Config.h"
 
 namespace facebook::velox::core {
@@ -90,7 +91,7 @@ void QueryCtx::updateSpilledBytesAndCheckLimit(uint64_t bytes) {
 void QueryCtx::updateTracedBytesAndCheckLimit(uint64_t bytes) {
   if (numTracedBytes_.fetch_add(bytes) + bytes >=
       queryConfig_.queryTraceMaxBytes()) {
-    VELOX_SPILL_LIMIT_EXCEEDED(fmt::format(
+    VELOX_TRACE_LIMIT_EXCEEDED(fmt::format(
         "Query exceeded per-query local trace limit of {}",
         succinctBytes(queryConfig_.queryTraceMaxBytes())));
   }

diff --git a/velox/exec/CMakeLists.txt b/velox/exec/CMakeLists.txt
@@ -63,7 +63,6 @@ velox_add_library(
   TaskTraceReader.cpp
   TaskTraceWriter.cpp
   Trace.cpp
-  TraceConfig.cpp
   TraceUtil.cpp
   PartitionedOutput.cpp
   PartitionFunction.cpp

diff --git a/velox/exec/Driver.h b/velox/exec/Driver.h
@@ -24,10 +24,10 @@
 
 #include "velox/common/base/Counters.h"
 #include "velox/common/base/StatsReporter.h"
+#include "velox/common/base/TraceConfig.h"
 #include "velox/common/time/CpuWallTimer.h"
 #include "velox/core/PlanFragment.h"
 #include "velox/core/QueryCtx.h"
-#include "velox/exec/TraceConfig.h"
 
 namespace facebook::velox::exec {
 

diff --git a/velox/exec/OperatorTraceWriter.h b/velox/exec/OperatorTraceWriter.h
@@ -16,7 +16,7 @@
 
 #pragma once
 
-#include "TraceConfig.h"
+#include "velox/common/base/TraceConfig.h"
 #include "velox/common/file/File.h"
 #include "velox/common/file/FileSystems.h"
 #include "velox/exec/Split.h"

diff --git a/velox/exec/Task.h b/velox/exec/Task.h
@@ -16,6 +16,7 @@
 #pragma once
 
 #include "velox/common/base/SkewedPartitionBalancer.h"
+#include "velox/common/base/TraceConfig.h"
 #include "velox/core/PlanFragment.h"
 #include "velox/core/QueryCtx.h"
 #include "velox/exec/Driver.h"
@@ -27,7 +28,6 @@
 #include "velox/exec/TaskStats.h"
 #include "velox/exec/TaskStructs.h"
 #include "velox/exec/TaskTraceWriter.h"
-#include "velox/exec/TraceConfig.h"
 #include "velox/vector/ComplexVector.h"
 
 namespace facebook::velox::exec {

diff --git a/velox/exec/Trace.h b/velox/exec/Trace.h
@@ -62,12 +62,4 @@ struct OperatorTraceSummary {
   std::string toString() const;
 };
 
-#define VELOX_TRACE_LIMIT_EXCEEDED(errorMessage)                    \
-  _VELOX_THROW(                                                     \
-      ::facebook::velox::VeloxRuntimeError,                         \
-      ::facebook::velox::error_source::kErrorSourceRuntime.c_str(), \
-      ::facebook::velox::error_code::kTraceLimitExceeded.c_str(),   \
-      /* isRetriable */ true,                                       \
-      "{}",                                                         \
-      errorMessage);
 } // namespace facebook::velox::exec::trace
diff --git a/velox/exec/tests/OperatorTraceTest.cpp b/velox/exec/tests/OperatorTraceTest.cpp
@@ -461,7 +461,7 @@ TEST_F(OperatorTraceTest, error) {
             .copyResults(pool()),
         "Query trace enabled but the trace dir is not set");
   }
-  // Duplicate trace plan node ids.
+  // No trace task regexp.
   {
     const auto queryConfigs = std::unordered_map<std::string, std::string>{
         {core::QueryConfig::kQueryTraceEnabled, "true"},