diff --git a/velox/buffer/tests/BufferTest.cpp b/velox/buffer/tests/BufferTest.cpp index d0eac34b74994..551b215855906 100644 --- a/velox/buffer/tests/BufferTest.cpp +++ b/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 @@ -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, ""); static_assert(Buffer::is_pod_like_v, ""); @@ -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(bufferPtr, 5, 6, nullptr), "Pool must not be null."); } -} // namespace velox -} // namespace facebook +} // namespace facebook::velox diff --git a/velox/buffer/tests/CMakeLists.txt b/velox/buffer/tests/CMakeLists.txt index 097f5ee3bb3f4..9e2bfaa4b9fcd 100644 --- a/velox/buffer/tests/CMakeLists.txt +++ b/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 diff --git a/velox/common/base/CMakeLists.txt b/velox/common/base/CMakeLists.txt index 0d9d2521b93af..3bde1734d28fc 100644 --- a/velox/common/base/CMakeLists.txt +++ b/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 diff --git a/velox/common/base/VeloxException.cpp b/velox/common/base/VeloxException.cpp index 82af2ba8a7d43..5d1a6bf45f922 100644 --- a/velox/common/base/VeloxException.cpp +++ b/velox/common/base/VeloxException.cpp @@ -15,6 +15,7 @@ */ #include "velox/common/base/VeloxException.h" +#include "velox/common/config/GlobalConfig.h" #include #include @@ -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; diff --git a/velox/common/base/VeloxException.h b/velox/common/base/VeloxException.h index 148ae38f06a2c..5ed38f6450393 100644 --- a/velox/common/base/VeloxException.h +++ b/velox/common/base/VeloxException.h @@ -23,7 +23,6 @@ #include #include #include -#include #include #include "velox/common/process/StackTrace.h" diff --git a/velox/common/base/tests/CMakeLists.txt b/velox/common/base/tests/CMakeLists.txt index 7d85566bf4ed4..3032eea49c5a4 100644 --- a/velox/common/base/tests/CMakeLists.txt +++ b/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 diff --git a/velox/common/base/tests/ExceptionTest.cpp b/velox/common/base/tests/ExceptionTest.cpp index 9386b8cb672e0..0d279caf446fd 100644 --- a/velox/common/base/tests/ExceptionTest.cpp +++ b/velox/common/base/tests/ExceptionTest.cpp @@ -19,6 +19,8 @@ #include #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); diff --git a/velox/common/config/CMakeLists.txt b/velox/common/config/CMakeLists.txt index 7780665a29251..94404bed10f19 100644 --- a/velox/common/config/CMakeLists.txt +++ b/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) + diff --git a/velox/common/config/Config.cpp b/velox/common/config/Config.cpp index ed1df2eb2df08..0582b1e755e40 100644 --- a/velox/common/config/Config.cpp +++ b/velox/common/config/Config.cpp @@ -17,12 +17,9 @@ #include #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: diff --git a/velox/common/config/GlobalConfig.cpp b/velox/common/config/GlobalConfig.cpp new file mode 100644 index 0000000000000..823d941572f82 --- /dev/null +++ b/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 diff --git a/velox/common/config/GlobalConfig.h b/velox/common/config/GlobalConfig.h index 26de8d36124f0..1ccd8e2bed271 100644 --- a/velox/common/config/GlobalConfig.h +++ b/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 diff --git a/velox/common/memory/tests/MemoryAllocatorTest.cpp b/velox/common/memory/tests/MemoryAllocatorTest.cpp index f83b657270f2d..02fcefebf5764 100644 --- a/velox/common/memory/tests/MemoryAllocatorTest.cpp +++ b/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 #include @@ -57,7 +58,7 @@ class MemoryAllocatorTest : public testing::TestWithParam { 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 = std::make_unique(); 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& 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) { diff --git a/velox/common/memory/tests/MemoryCapExceededTest.cpp b/velox/common/memory/tests/MemoryCapExceededTest.cpp index a1d4c695882bb..0518f6da49cd4 100644 --- a/velox/common/memory/tests/MemoryCapExceededTest.cpp +++ b/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 @@ -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(); } }; diff --git a/velox/common/memory/tests/MemoryManagerTest.cpp b/velox/common/memory/tests/MemoryManagerTest.cpp index cbfa8875ff2e4..cbbf33b389b51 100644 --- a/velox/common/memory/tests/MemoryManagerTest.cpp +++ b/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); diff --git a/velox/dwio/dwrf/test/CMakeLists.txt b/velox/dwio/dwrf/test/CMakeLists.txt index edd5f56cb77ff..615411c4eb0dd 100644 --- a/velox/dwio/dwrf/test/CMakeLists.txt +++ b/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 diff --git a/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp b/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp index 16ecca5f3663c..83d38e6deb2a9 100644 --- a/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp +++ b/velox/dwio/dwrf/test/TestIntegerDictionaryEncoder.cpp @@ -17,6 +17,7 @@ #include #include #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({}); } }; diff --git a/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp b/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp index e547f7d16d9ca..3ad8ddfc73616 100644 --- a/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp +++ b/velox/dwio/dwrf/test/TestStringDictionaryEncoder.cpp @@ -17,6 +17,7 @@ #include #include #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({}); } }; diff --git a/velox/exec/tests/utils/CMakeLists.txt b/velox/exec/tests/utils/CMakeLists.txt index 294493367012a..97c31789b95ba 100644 --- a/velox/exec/tests/utils/CMakeLists.txt +++ b/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}) diff --git a/velox/exec/tests/utils/OperatorTestBase.cpp b/velox/exec/tests/utils/OperatorTestBase.cpp index 33ce9c6603b98..14ede4c70bf89 100644 --- a/velox/exec/tests/utils/OperatorTestBase.cpp +++ b/velox/exec/tests/utils/OperatorTestBase.cpp @@ -22,6 +22,7 @@ #include "velox/common/memory/SharedArbitrator.h" #include "velox/common/testutil/TestValue.h" #include "velox/exec/tests/utils/LocalExchangeSource.h" +#include "velox/flag_definitions/flags.h" #include "velox/functions/prestosql/aggregates/RegisterAggregateFunctions.h" #include "velox/functions/prestosql/registration/RegistrationFunctions.h" #include "velox/parse/Expressions.h" @@ -63,7 +64,7 @@ OperatorTestBase::~OperatorTestBase() { void OperatorTestBase::SetUpTestCase() { FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true; FLAGS_velox_memory_leak_check_enabled = true; - config::translateFlagsToGlobalConfig(); + translateFlagsToGlobalConfig(); memory::SharedArbitrator::registerFactory(); resetMemory(); functions::prestosql::registerAllScalarFunctions(); diff --git a/velox/flag_definitions/CMakeLists.txt b/velox/flag_definitions/CMakeLists.txt index 8569cc3f997d8..24d4d2b32bfb2 100644 --- a/velox/flag_definitions/CMakeLists.txt +++ b/velox/flag_definitions/CMakeLists.txt @@ -12,4 +12,4 @@ # See the License for the specific language governing permissions and # limitations under the License. velox_add_library(velox_flag_definitions OBJECT flags.cpp) -velox_link_libraries(velox_flag_definitions PRIVATE gflags::gflags) +velox_link_libraries(velox_flag_definitions PRIVATE velox_global_config gflags::gflags) diff --git a/velox/flag_definitions/flags.cpp b/velox/flag_definitions/flags.cpp index 17d5aae2dbc6c..a92e63ebbb360 100644 --- a/velox/flag_definitions/flags.cpp +++ b/velox/flag_definitions/flags.cpp @@ -109,7 +109,7 @@ DEFINE_bool( "Whether allow to memory capacity transfer between memory pools from " "different tasks, which might happen in use case like Spark-Gluten"); -namespace facebook::velox::config { +namespace facebook::velox { void translateFlagsToGlobalConfig() { config::globalConfig.memoryNumSharedLeafPools = FLAGS_velox_memory_num_shared_leaf_pools; @@ -125,5 +125,13 @@ void translateFlagsToGlobalConfig() { FLAGS_velox_suppress_memory_capacity_exceeding_error_message; config::globalConfig.memoryPoolCapacityTransferAcrossTasks = FLAGS_velox_memory_pool_capacity_transfer_across_tasks; + config::globalConfig.exceptionSystemStacktraceEnabled = + FLAGS_velox_exception_system_stacktrace_enabled; + config::globalConfig.exceptionSystemStacktraceRateLimitMs = + FLAGS_velox_exception_system_stacktrace_rate_limit_ms; + config::globalConfig.exceptionUserStacktraceEnabled = + FLAGS_velox_exception_user_stacktrace_enabled; + config::globalConfig.exceptionUserStacktraceRateLimitMs = + FLAGS_velox_exception_user_stacktrace_rate_limit_ms; } -} // namespace facebook::velox::config +} // namespace facebook::velox diff --git a/velox/flag_definitions/flags.h b/velox/flag_definitions/flags.h new file mode 100644 index 0000000000000..4b748602adae6 --- /dev/null +++ b/velox/flag_definitions/flags.h @@ -0,0 +1,19 @@ +/* + * 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. + */ + +namespace facebook::velox { +void translateFlagsToGlobalConfig(); +}