misc: Revert GlobalConfig to use gflags again

velox/buffer/tests/BufferTest.cpp

@@ -19,7 +19,6 @@
 #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>
@@ -28,7 +27,8 @@
 
 DECLARE_bool(velox_enable_memory_usage_track_in_default_memory_pool);
 
-namespace facebook::velox {
+namespace facebook {
+namespace velox {
 
 static_assert(Buffer::is_pod_like_v<int64_t>, "");
 static_assert(Buffer::is_pod_like_v<StringView>, "");
@@ -40,7 +40,6 @@ class BufferTest : public testing::Test {
  protected:
   static void SetUpTestCase() {
     FLAGS_velox_enable_memory_usage_track_in_default_memory_pool = true;
-    translateFlagsToGlobalConfig();
   }
 
   void SetUp() override {
@@ -491,4 +490,5 @@ TEST_F(BufferTest, sliceBooleanBuffer) {
       Buffer::slice<bool>(bufferPtr, 5, 6, nullptr), "Pool must not be null.");
 }
 
-} // namespace facebook::velox
+} // namespace velox
+} // namespace facebook

velox/buffer/tests/CMakeLists.txt

@@ -20,7 +20,6 @@ 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,8 +14,14 @@
 
 velox_add_library(velox_exception Exceptions.cpp VeloxException.cpp
                   Exceptions.h)
-velox_link_libraries(velox_exception PUBLIC velox_process Folly::folly fmt::fmt
-                                            glog::glog)
+velox_link_libraries(
+  velox_exception
+  PUBLIC velox_flag_definitions
+         velox_process
+         Folly::folly
+         fmt::fmt
+         gflags::gflags
+         glog::glog)
 
 velox_add_library(
   velox_common_base

velox/common/base/VeloxException.cpp

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

velox/common/base/VeloxException.h

@@ -23,6 +23,7 @@
 #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,7 +49,6 @@ 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,8 +19,6 @@
 #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;
 
@@ -85,7 +83,6 @@ 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(
@@ -112,8 +109,8 @@ void testExceptionTraceCollectionControl(bool userException, bool enabled) {
     SCOPED_TRACE(fmt::format(
         "enabled: {}, user flag: {}, sys flag: {}",
         enabled,
-        config::globalConfig().exceptionUserStacktraceEnabled,
-        config::globalConfig().exceptionSystemStacktraceEnabled));
+        FLAGS_velox_exception_user_stacktrace_enabled,
+        FLAGS_velox_exception_system_stacktrace_enabled));
     ASSERT_EQ(userException, e.exceptionType() == VeloxException::Type::kUser);
     ASSERT_EQ(enabled, e.stackTrace() != nullptr);
   }
@@ -126,7 +123,6 @@ 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;
@@ -150,7 +146,6 @@ void testExceptionTraceCollectionRateControl(
     FLAGS_velox_exception_user_stacktrace_rate_limit_ms =
         folly::Random::rand32();
   }
-  translateFlagsToGlobalConfig();
   for (int iter = 0; iter < 3; ++iter) {
     try {
       if (userException) {
@@ -179,8 +174,8 @@ void testExceptionTraceCollectionRateControl(
           "userException: {}, hasRateLimit: {}, user limit: {}ms, sys limit: {}ms",
           userException,
           hasRateLimit,
-          config::globalConfig().exceptionUserStacktraceRateLimitMs,
-          config::globalConfig().exceptionSystemStacktraceRateLimitMs));
+          FLAGS_velox_exception_user_stacktrace_rate_limit_ms,
+          FLAGS_velox_exception_system_stacktrace_rate_limit_ms));
       ASSERT_EQ(
           userException, e.exceptionType() == VeloxException::Type::kUser);
       ASSERT_EQ(!hasRateLimit || ((iter % 2) == 0), e.stackTrace() != nullptr);

velox/common/caching/SsdFile.cpp

@@ -22,7 +22,6 @@
 #include "velox/common/base/SuccinctPrinter.h"
 #include "velox/common/caching/FileIds.h"
 #include "velox/common/caching/SsdCache.h"
-#include "velox/common/config/GlobalConfig.h"
 #include "velox/common/memory/Memory.h"
 #include "velox/common/process/TraceContext.h"
 
@@ -35,6 +34,9 @@
 #include <sys/types.h>
 #include <numeric>
 
+DECLARE_bool(velox_ssd_odirect);
+DECLARE_bool(velox_ssd_verify_write);
+
 namespace facebook::velox::cache {
 
 namespace {
@@ -116,7 +118,7 @@ SsdFile::SsdFile(const Config& config)
   process::TraceContext trace("SsdFile::SsdFile");
   filesystems::FileOptions fileOptions;
   fileOptions.shouldThrowOnFileAlreadyExists = false;
-  fileOptions.bufferIo = !config::globalConfig().useSsdODirect;
+  fileOptions.bufferIo = !FLAGS_velox_ssd_odirect;
   writeFile_ = fs_->openFileForWrite(fileName_, fileOptions);
   readFile_ = fs_->openFileForRead(fileName_, fileOptions);
 
@@ -415,7 +417,7 @@ void SsdFile::write(std::vector<CachePin>& pins) {
           checksum = checksumEntry(*entry);
         }
         entries_[std::move(key)] = SsdRun(offset, size, checksum);
-        if (config::globalConfig().verifySsdWrite) {
+        if (FLAGS_velox_ssd_verify_write) {
           verifyWrite(*entry, SsdRun(offset, size, checksum));
         }
         offset += size;

velox/common/caching/tests/AsyncDataCacheTest.cpp

@@ -27,7 +27,6 @@
 #include "velox/common/testutil/ScopedTestTime.h"
 #include "velox/common/testutil/TestValue.h"
 #include "velox/exec/tests/utils/TempDirectoryPath.h"
-#include "velox/flag_definitions/flags.h"
 
 #include <folly/executors/IOThreadPoolExecutor.h>
 #include <folly/executors/QueuedImmediateExecutor.h>
@@ -43,6 +42,9 @@ using namespace facebook::velox::common::testutil;
 
 using facebook::velox::memory::MemoryAllocator;
 
+DECLARE_bool(velox_ssd_odirect);
+DECLARE_bool(velox_ssd_verify_write);
+
 // Represents a planned load from a file. Many of these constitute a load plan.
 struct Request {
   Request(uint64_t _offset, uint64_t _size) : offset(_offset), size(_size) {}
@@ -138,7 +140,6 @@ class AsyncDataCacheTest : public ::testing::TestWithParam<TestParam> {
     if (ssdBytes > 0) {
       // tmpfs does not support O_DIRECT, so turn this off for testing.
       FLAGS_velox_ssd_odirect = false;
-      translateFlagsToGlobalConfig();
       // Make a new tempDirectory only if one is not already set. The
       // second creation of cache must find the checkpoint of the
       // previous one.
@@ -720,7 +721,7 @@ TEST_P(AsyncDataCacheTest, pin) {
 
 TEST_P(AsyncDataCacheTest, replace) {
   constexpr int64_t kMaxBytes = 64 << 20;
-  config::globalConfig().exceptionUserStacktraceEnabled = false;
+  FLAGS_velox_exception_user_stacktrace_enabled = false;
   initializeCache(kMaxBytes);
   // Load 10x the max size, inject an error every 21 batches.
   loadLoop(0, kMaxBytes * 10, 21);
@@ -738,7 +739,7 @@ TEST_P(AsyncDataCacheTest, replace) {
 
 TEST_P(AsyncDataCacheTest, evictAccounting) {
   constexpr int64_t kMaxBytes = 64 << 20;
-  config::globalConfig().exceptionUserStacktraceEnabled = false;
+  FLAGS_velox_exception_user_stacktrace_enabled = false;
   initializeCache(kMaxBytes);
   auto pool = manager_->addLeafPool("test");
 
@@ -762,7 +763,7 @@ TEST_P(AsyncDataCacheTest, evictAccounting) {
 TEST_P(AsyncDataCacheTest, largeEvict) {
   constexpr int64_t kMaxBytes = 256 << 20;
   constexpr int32_t kNumThreads = 24;
-  config::globalConfig().exceptionUserStacktraceEnabled = false;
+  FLAGS_velox_exception_user_stacktrace_enabled = false;
   initializeCache(kMaxBytes);
   // Load 10x the max size, inject an allocation of 1/8 the capacity every 4
   // batches.
@@ -841,15 +842,14 @@ TEST_P(AsyncDataCacheTest, DISABLED_ssd) {
   constexpr uint64_t kRamBytes = 32 << 20;
   constexpr uint64_t kSsdBytes = 512UL << 20;
 #endif
-  config::globalConfig().exceptionUserStacktraceEnabled = false;
+  FLAGS_velox_exception_user_stacktrace_enabled = false;
   initializeCache(kRamBytes, kSsdBytes);
   cache_->setVerifyHook(
       [&](const AsyncDataCacheEntry& entry) { checkContents(entry); });
 
   // Read back all writes. This increases the chance of writes falling behind
   // new entry creation.
   FLAGS_velox_ssd_verify_write = true;
-  translateFlagsToGlobalConfig();
 
   // We read kSsdBytes worth of data on 16 threads. The same data will be hit by
   // all threads. The expectation is that most of the data ends up on SSD. All
@@ -865,7 +865,7 @@ TEST_P(AsyncDataCacheTest, DISABLED_ssd) {
 
   // We allow writes to proceed faster.
   FLAGS_velox_ssd_verify_write = false;
-  translateFlagsToGlobalConfig();
+
   // We read the data back. The verify hook checks correct values. Error every
   // 13 batch loads.
   runThreads(16, [&](int32_t /*i*/) { loadLoop(0, kSsdBytes, 13); });

velox/common/caching/tests/SsdFileTest.cpp

@@ -17,7 +17,6 @@
 #include "velox/common/base/tests/GTestUtils.h"
 #include "velox/common/caching/FileIds.h"
 #include "velox/common/caching/tests/CacheTestUtil.h"
-#include "velox/common/config/GlobalConfig.h"
 #include "velox/common/file/FileSystems.h"
 #include "velox/common/file/tests/FaultyFileSystem.h"
 #include "velox/common/memory/Memory.h"
@@ -36,6 +35,9 @@ using namespace facebook::velox::tests::utils;
 
 using facebook::velox::memory::MemoryAllocator;
 
+DECLARE_bool(velox_ssd_odirect);
+DECLARE_bool(velox_ssd_verify_write);
+
 // Represents an entry written to SSD.
 struct TestEntry {
   FileCacheKey key;
@@ -74,7 +76,7 @@ class SsdFileTest : public testing::Test {
       bool disableFileCow = false,
       bool enableFaultInjection = false) {
     // tmpfs does not support O_DIRECT, so turn this off for testing.
-    config::globalConfig().useSsdODirect = false;
+    FLAGS_velox_ssd_odirect = false;
     cache_ = AsyncDataCache::create(memory::memoryManager()->allocator());
     cacheHelper_ =
         std::make_unique<test::AsyncDataCacheTestHelper>(cache_.get());
@@ -328,7 +330,7 @@ TEST_F(SsdFileTest, writeAndRead) {
   constexpr int64_t kSsdSize = 16 * SsdFile::kRegionSize;
   std::vector<TestEntry> allEntries;
   initializeCache(kSsdSize);
-  config::globalConfig().verifySsdWrite = true;
+  FLAGS_velox_ssd_verify_write = true;
   for (auto startOffset = 0; startOffset <= kSsdSize - SsdFile::kRegionSize;
        startOffset += SsdFile::kRegionSize) {
     auto pins =
@@ -403,7 +405,7 @@ TEST_F(SsdFileTest, checkpoint) {
   constexpr int64_t kSsdSize = 16 * SsdFile::kRegionSize;
   const uint64_t checkpointIntervalBytes = 5 * SsdFile::kRegionSize;
   const auto fileNameAlt = StringIdLease(fileIds(), "fileInStorageAlt");
-  config::globalConfig().verifySsdWrite = true;
+  FLAGS_velox_ssd_verify_write = true;
   initializeCache(kSsdSize, checkpointIntervalBytes);
 
   std::vector<TestEntry> allEntries;
@@ -499,7 +501,7 @@ TEST_F(SsdFileTest, checkpoint) {
 TEST_F(SsdFileTest, fileCorruption) {
   constexpr int64_t kSsdSize = 16 * SsdFile::kRegionSize;
   const uint64_t checkpointIntervalBytes = 5 * SsdFile::kRegionSize;
-  config::globalConfig().verifySsdWrite = true;
+  FLAGS_velox_ssd_verify_write = true;
 
   const auto populateCache = [&](std::vector<TestEntry>& entries) {
     entries.clear();
@@ -571,7 +573,7 @@ TEST_F(SsdFileTest, fileCorruption) {
 TEST_F(SsdFileTest, recoverFromCheckpointWithChecksum) {
   constexpr int64_t kSsdSize = 4 * SsdFile::kRegionSize;
   const uint64_t checkpointIntervalBytes = 3 * SsdFile::kRegionSize;
-  config::globalConfig().verifySsdWrite = true;
+  FLAGS_velox_ssd_verify_write = true;
 
   // Test if cache data can be recovered with different settings.
   struct {

velox/common/config/CMakeLists.txt

@@ -12,14 +12,13 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-if(${VELOX_BUILD_TESTING})
+if (${VELOX_BUILD_TESTING})
   add_subdirectory(tests)
-endif()
-
-velox_add_library(velox_global_config GlobalConfig.cpp)
+endif ()
 
 velox_add_library(velox_common_config Config.cpp)
 velox_link_libraries(
   velox_common_config
-  PUBLIC velox_common_base velox_global_config velox_exception
+  PUBLIC velox_common_base
+         velox_exception
   PRIVATE re2::re2)