address review comments

velox/buffer/tests/BufferTest.cpp

@@ -19,6 +19,7 @@
 #include "folly/Range.h"
 #include "velox/common/base/tests/GTestUtils.h"
 #include "velox/common/testutil/TestValue.h"
+#include "velox/flag_definitions/flags.h"
 #include "velox/type/StringView.h"
 
 #include <glog/logging.h>
@@ -27,8 +28,7 @@
 
 DECLARE_bool(velox_enable_memory_usage_track_in_default_memory_pool);
 
-namespace facebook {
-namespace velox {
+namespace facebook::velox {
 
 static_assert(Buffer::is_pod_like_v<int64_t>, "");
 static_assert(Buffer::is_pod_like_v<StringView>, "");
@@ -40,7 +40,7 @@ class BufferTest : public testing::Test {
  protected:
   static void SetUpTestCase() {
     FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true;
-    config::translateFlagsToGlobalConfig();
+    translateFlagsToGlobalConfig();
   }
 
   void SetUp() override {
@@ -491,5 +491,4 @@ TEST_F(BufferTest, sliceBooleanBuffer) {
       Buffer::slice<bool>(bufferPtr, 5, 6, nullptr), "Pool must not be null.");
 }
 
-} // namespace velox
-} // namespace facebook
+} // namespace facebook::velox

velox/buffer/tests/CMakeLists.txt

@@ -20,6 +20,7 @@ target_link_libraries(
   velox_memory
   velox_buffer
   velox_test_util
+  velox_flag_definitions
   GTest::gtest
   GTest::gtest_main
   GTest::gmock

velox/common/base/CMakeLists.txt

@@ -14,14 +14,8 @@
 
 velox_add_library(velox_exception Exceptions.cpp VeloxException.cpp
                   Exceptions.h)
-velox_link_libraries(
-  velox_exception
-  PUBLIC velox_process
-         velox_flag_definitions
-         Folly::folly
-         fmt::fmt
-         gflags::gflags
-         glog::glog)
+velox_link_libraries(velox_exception PUBLIC velox_process Folly::folly fmt::fmt
+                                            glog::glog)
 
 velox_add_library(
   velox_common_base

velox/common/base/VeloxException.cpp

@@ -15,6 +15,7 @@
  */
 
 #include "velox/common/base/VeloxException.h"
+#include "velox/common/config/GlobalConfig.h"
 
 #include <folly/synchronization/AtomicStruct.h>
 #include <exception>
@@ -134,16 +135,18 @@ namespace {
 bool isStackTraceEnabled(VeloxException::Type type) {
   using namespace std::literals::chrono_literals;
   const bool isSysException = type == VeloxException::Type::kSystem;
-  if ((isSysException && !FLAGS_velox_exception_system_stacktrace_enabled) ||
-      (!isSysException && !FLAGS_velox_exception_user_stacktrace_enabled)) {
+  if ((isSysException &&
+       !config::globalConfig.exceptionSystemStacktraceEnabled) ||
+      (!isSysException &&
+       !config::globalConfig.exceptionUserStacktraceEnabled)) {
     // VeloxException stacktraces are disabled.
     return false;
   }
 
   const int32_t rateLimitMs = isSysException
-      ? FLAGS_velox_exception_system_stacktrace_rate_limit_ms
-      : FLAGS_velox_exception_user_stacktrace_rate_limit_ms;
-  // not static so the gflag can be manipulated at runtime
+      ? config::globalConfig.exceptionSystemStacktraceRateLimitMs
+      : config::globalConfig.exceptionUserStacktraceRateLimitMs;
+  // not static so the global config can be manipulated at runtime
   if (0 == rateLimitMs) {
     // VeloxException stacktraces are not rate-limited
     return true;

velox/common/base/VeloxException.h

@@ -23,7 +23,6 @@
 #include <folly/FixedString.h>
 #include <folly/String.h>
 #include <folly/synchronization/CallOnce.h>
-#include <gflags/gflags.h>
 #include <glog/logging.h>
 
 #include "velox/common/process/StackTrace.h"

velox/common/base/tests/CMakeLists.txt

@@ -49,6 +49,7 @@ target_link_libraries(
     velox_status
     velox_exception
     velox_temp_path
+    velox_flag_definitions
     Boost::filesystem
     Boost::headers
     Folly::folly

velox/common/base/tests/ExceptionTest.cpp

@@ -19,6 +19,8 @@
 #include <gtest/gtest.h>
 
 #include "velox/common/base/Exceptions.h"
+#include "velox/common/config/GlobalConfig.h"
+#include "velox/flag_definitions/flags.h"
 
 using namespace facebook::velox;
 
@@ -83,6 +85,7 @@ void testExceptionTraceCollectionControl(bool userException, bool enabled) {
     FLAGS_velox_exception_system_stacktrace_enabled = enabled ? true : false;
     FLAGS_velox_exception_user_stacktrace_enabled = folly::Random::oneIn(2);
   }
+  translateFlagsToGlobalConfig();
   try {
     if (userException) {
       throw VeloxUserError(
@@ -109,8 +112,8 @@ void testExceptionTraceCollectionControl(bool userException, bool enabled) {
     SCOPED_TRACE(fmt::format(
         "enabled: {}, user flag: {}, sys flag: {}",
         enabled,
-        FLAGS_velox_exception_user_stacktrace_enabled,
-        FLAGS_velox_exception_system_stacktrace_enabled));
+        config::globalConfig.exceptionUserStacktraceEnabled,
+        config::globalConfig.exceptionSystemStacktraceEnabled));
     ASSERT_EQ(userException, e.exceptionType() == VeloxException::Type::kUser);
     ASSERT_EQ(enabled, e.stackTrace() != nullptr);
   }
@@ -123,6 +126,7 @@ void testExceptionTraceCollectionRateControl(
   // Enable trace rate control in the test.
   FLAGS_velox_exception_user_stacktrace_enabled = true;
   FLAGS_velox_exception_system_stacktrace_enabled = true;
+  translateFlagsToGlobalConfig();
   // Set rate control interval to a large value to avoid time related test
   // flakiness.
   const int kRateLimitIntervalMs = 4000;
@@ -146,6 +150,7 @@ void testExceptionTraceCollectionRateControl(
     FLAGS_velox_exception_user_stacktrace_rate_limit_ms =
         folly::Random::rand32();
   }
+  translateFlagsToGlobalConfig();
   for (int iter = 0; iter < 3; ++iter) {
     try {
       if (userException) {
@@ -174,8 +179,8 @@ void testExceptionTraceCollectionRateControl(
           "userException: {}, hasRateLimit: {}, user limit: {}ms, sys limit: {}ms",
           userException,
           hasRateLimit,
-          FLAGS_velox_exception_user_stacktrace_rate_limit_ms,
-          FLAGS_velox_exception_system_stacktrace_rate_limit_ms));
+          config::globalConfig.exceptionUserStacktraceRateLimitMs,
+          config::globalConfig.exceptionSystemStacktraceRateLimitMs));
       ASSERT_EQ(
           userException, e.exceptionType() == VeloxException::Type::kUser);
       ASSERT_EQ(!hasRateLimit || ((iter % 2) == 0), e.stackTrace() != nullptr);

velox/common/config/CMakeLists.txt

@@ -16,9 +16,13 @@ if (${VELOX_BUILD_TESTING})
   add_subdirectory(tests)
 endif ()
 
+velox_add_library(velox_global_config OBJECT GlobalConfig.cpp)
+
 velox_add_library(velox_common_config Config.cpp)
 velox_link_libraries(
   velox_common_config
   PUBLIC velox_common_base
+         velox_global_config
          velox_exception
   PRIVATE re2::re2)
+

velox/common/config/Config.cpp

@@ -17,12 +17,9 @@
 #include <re2/re2.h>
 
 #include "velox/common/config/Config.h"
-#include "velox/common/config/GlobalConfig.h"
 
 namespace facebook::velox::config {
 
-GlobalConfiguration globalConfig;
-
 double toBytesPerCapacityUnit(CapacityUnit unit) {
   switch (unit) {
     case CapacityUnit::BYTE:

velox/common/config/GlobalConfig.cpp

@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include "velox/common/config/GlobalConfig.h"
+
+namespace facebook::velox::config {
+
+GlobalConfiguration globalConfig;
+
+} // namespace facebook::velox::config

velox/common/config/GlobalConfig.h

@@ -43,10 +43,18 @@ struct GlobalConfiguration {
   /// Whether allow to memory capacity transfer between memory pools from
   /// different tasks, which might happen in use case like Spark-Gluten
   bool memoryPoolCapacityTransferAcrossTasks{false};
+  /// Enable the stacktrace for system type of VeloxException.
+  bool exceptionSystemStacktraceEnabled{true};
+  /// Enable the stacktrace for user type of VeloxException.
+  bool exceptionUserStacktraceEnabled{false};
+  /// Min time interval in milliseconds between stack traces captured in
+  /// user type of VeloxException; off when set to 0 (the default).
+  int32_t exceptionUserStacktraceRateLimitMs{0};
+  /// Min time interval in milliseconds between stack traces captured in
+  /// system type of VeloxException; off when set to 0 (the default).
+  int32_t exceptionSystemStacktraceRateLimitMs{0};
 };
 
 extern GlobalConfiguration globalConfig;
 
-void translateFlagsToGlobalConfig();
-
 } // namespace facebook::velox::config

velox/common/memory/tests/MemoryAllocatorTest.cpp

@@ -22,6 +22,7 @@
 #include "velox/common/memory/MmapArena.h"
 #include "velox/common/memory/SharedArbitrator.h"
 #include "velox/common/testutil/TestValue.h"
+#include "velox/flag_definitions/flags.h"
 
 #include <fmt/format.h>
 #include <folly/Random.h>
@@ -57,7 +58,7 @@ class MemoryAllocatorTest : public testing::TestWithParam<int> {
   static void SetUpTestCase() {
     TestValue::enable();
     FLAGS_velox_memory_leak_check_enabled = true;
-    config::translateFlagsToGlobalConfig();
+    translateFlagsToGlobalConfig();
   }
 
   void SetUp() override {
@@ -653,7 +654,7 @@ TEST_P(MemoryAllocatorTest, stats) {
   }
 
   FLAGS_velox_time_allocations = true;
-  config::translateFlagsToGlobalConfig();
+  translateFlagsToGlobalConfig();
   for (auto i = 0; i < sizes.size(); ++i) {
     std::unique_ptr<Allocation> allocation = std::make_unique<Allocation>();
     auto size = sizes[i];
@@ -666,7 +667,7 @@ TEST_P(MemoryAllocatorTest, stats) {
     ASSERT_GE(stats.sizes[i].numAllocations, 1);
   }
   FLAGS_velox_time_allocations = false;
-  config::translateFlagsToGlobalConfig();
+  translateFlagsToGlobalConfig();
 }
 
 TEST_P(MemoryAllocatorTest, singleAllocation) {
@@ -675,7 +676,7 @@ TEST_P(MemoryAllocatorTest, singleAllocation) {
   }
 
   FLAGS_velox_time_allocations = true;
-  config::translateFlagsToGlobalConfig();
+  translateFlagsToGlobalConfig();
   const std::vector<MachinePageCount>& sizes = instance_->sizeClasses();
   MachinePageCount capacity = kCapacityPages;
   for (auto i = 0; i < sizes.size(); ++i) {
@@ -723,7 +724,7 @@ TEST_P(MemoryAllocatorTest, singleAllocation) {
     ASSERT_TRUE(instance_->checkConsistency());
   }
   FLAGS_velox_time_allocations = false;
-  config::translateFlagsToGlobalConfig();
+  translateFlagsToGlobalConfig();
 }
 
 TEST_P(MemoryAllocatorTest, increasingSize) {

velox/common/memory/tests/MemoryCapExceededTest.cpp

@@ -19,6 +19,7 @@
 #include "velox/common/memory/MmapAllocator.h"
 #include "velox/exec/tests/utils/OperatorTestBase.h"
 #include "velox/exec/tests/utils/PlanBuilder.h"
+#include "velox/flag_definitions/flags.h"
 
 #include <re2/re2.h>
 
@@ -34,13 +35,13 @@ class MemoryCapExceededTest : public OperatorTestBase,
     // NOTE: if 'GetParam()' is true, then suppress the verbose error message in
     // memory capacity exceeded exception.
     FLAGS_velox_suppress_memory_capacity_exceeding_error_message = GetParam();
-    config::translateFlagsToGlobalConfig();
+    translateFlagsToGlobalConfig();
   }
 
   void TearDown() override {
     OperatorTestBase::TearDown();
     FLAGS_velox_suppress_memory_capacity_exceeding_error_message = false;
-    config::translateFlagsToGlobalConfig();
+    translateFlagsToGlobalConfig();
   }
 };
 

velox/common/memory/tests/MemoryManagerTest.cpp

@@ -24,6 +24,7 @@
 #include "velox/common/memory/MallocAllocator.h"
 #include "velox/common/memory/Memory.h"
 #include "velox/common/memory/SharedArbitrator.h"
+#include "velox/flag_definitions/flags.h"
 
 DECLARE_int32(velox_memory_num_shared_leaf_pools);
 DECLARE_bool(velox_enable_memory_usage_track_in_default_memory_pool);
@@ -44,7 +45,7 @@ class MemoryManagerTest : public testing::Test {
  protected:
   static void SetUpTestCase() {
     SharedArbitrator::registerFactory();
-    config::translateFlagsToGlobalConfig();
+    translateFlagsToGlobalConfig();
   }
 
   inline static const std::string arbitratorKind_{"SHARED"};
@@ -371,7 +372,7 @@ TEST_F(MemoryManagerTest, defaultMemoryUsageTracking) {
   for (bool trackDefaultMemoryUsage : {false, true}) {
     FLAGS_velox_enable_memory_usage_track_in_default_memory_pool =
         trackDefaultMemoryUsage;
-    config::translateFlagsToGlobalConfig();
+    translateFlagsToGlobalConfig();
     MemoryManager manager{};
     auto defaultPool = manager.addLeafPool("defaultMemoryUsageTracking");
     ASSERT_EQ(defaultPool->trackUsage(), trackDefaultMemoryUsage);

velox/dwio/dwrf/test/CMakeLists.txt

@@ -20,6 +20,7 @@ set(TEST_LINK_LIBS
     velox_dwio_dwrf_reader
     velox_dwio_dwrf_writer
     velox_row_fast
+    velox_flag_definitions
     gflags::gflags
     GTest::gtest
     GTest::gtest_main

velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp

@@ -17,6 +17,7 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 #include "velox/dwio/dwrf/writer/IntegerDictionaryEncoder.h"
+#include "velox/flag_definitions/flags.h"
 
 DECLARE_bool(velox_enable_memory_usage_track_in_default_memory_pool);
 
@@ -29,7 +30,7 @@ class TestIntegerDictionaryEncoder : public ::testing::Test {
  protected:
   static void SetUpTestCase() {
     FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true;
-    config::translateFlagsToGlobalConfig();
+    translateFlagsToGlobalConfig();
     memory::MemoryManager::testingSetInstance({});
   }
 };

velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp

@@ -17,6 +17,7 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 #include "velox/dwio/dwrf/writer/StringDictionaryEncoder.h"
+#include "velox/flag_definitions/flags.h"
 
 DECLARE_bool(velox_enable_memory_usage_track_in_default_memory_pool);
 
@@ -28,7 +29,7 @@ class TestStringDictionaryEncoder : public ::testing::Test {
  protected:
   static void SetUpTestCase() {
     FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true;
-    config::translateFlagsToGlobalConfig();
+    translateFlagsToGlobalConfig();
     memory::MemoryManager::testingSetInstance({});
   }
 };

velox/exec/tests/utils/CMakeLists.txt

@@ -68,6 +68,7 @@ target_link_libraries(
   velox_tpch_connector
   velox_presto_serializer
   velox_functions_prestosql
+  velox_flag_definitions
   velox_aggregates)
 
 if(${VELOX_BUILD_RUNNER})