Change memory-reclaim-max-wait-time to max-memory-arbitration-time

tanjialiang · Nov 14, 2024 · c44d5f6 · c44d5f6
1 parent 99979c4
commit c44d5f6
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 8 deletions.
diff --git a/velox/common/memory/SharedArbitrator.cpp b/velox/common/memory/SharedArbitrator.cpp
@@ -94,6 +94,17 @@ uint64_t SharedArbitrator::ExtraConfig::memoryPoolReservedCapacity(
       config::CapacityUnit::BYTE);
 }
 
+uint64_t SharedArbitrator::ExtraConfig::maxMemoryArbitrationTimeNs(
+    const std::unordered_map<std::string, std::string>& configs) {
+  return std::chrono::duration_cast<std::chrono::nanoseconds>(
+             config::toDuration(getConfig<std::string>(
+                 configs,
+                 kMaxMemoryArbitrationTime,
+                 std::string(kDefaultMaxMemoryArbitrationTime))))
+      .count();
+}
+
+// TODO: Remove after name change complete
 uint64_t SharedArbitrator::ExtraConfig::memoryReclaimMaxWaitTimeNs(
     const std::unordered_map<std::string, std::string>& configs) {
   return std::chrono::duration_cast<std::chrono::nanoseconds>(
@@ -206,7 +217,7 @@ SharedArbitrator::SharedArbitrator(const Config& config)
       reservedCapacity_(ExtraConfig::reservedCapacity(config.extraConfigs)),
       checkUsageLeak_(ExtraConfig::checkUsageLeak(config.extraConfigs)),
       maxArbitrationTimeNs_(
-          ExtraConfig::memoryReclaimMaxWaitTimeNs(config.extraConfigs)),
+          ExtraConfig::maxMemoryArbitrationTimeNs(config.extraConfigs)),
       participantConfig_(
           ExtraConfig::memoryPoolInitialCapacity(config.extraConfigs),
           ExtraConfig::memoryPoolReservedCapacity(config.extraConfigs),

diff --git a/velox/common/memory/SharedArbitrator.h b/velox/common/memory/SharedArbitrator.h
@@ -81,6 +81,13 @@ class SharedArbitrator : public memory::MemoryArbitrator {
     /// the memory arbitration from getting stuck when the memory reclaim waits
     /// for a hanging query task to pause. If it is zero, then there is no
     /// timeout.
+    static constexpr std::string_view kMaxMemoryArbitrationTime{
+        "max-memory-arbitration-time"};
+    static constexpr std::string_view kDefaultMaxMemoryArbitrationTime{"5m"};
+    static uint64_t maxMemoryArbitrationTimeNs(
+        const std::unordered_map<std::string, std::string>& configs);
+
+    // TODO: Remove after name change complete
     static constexpr std::string_view kMemoryReclaimMaxWaitTime{
         "memory-reclaim-max-wait-time"};
     static constexpr std::string_view kDefaultMemoryReclaimMaxWaitTime{"5m"};

diff --git a/velox/common/memory/tests/MockSharedArbitratorTest.cpp b/velox/common/memory/tests/MockSharedArbitratorTest.cpp
@@ -479,7 +479,7 @@ class MockSharedArbitrationTest : public testing::Test {
          folly::to<std::string>(globalArbitrationReclaimPct)},
         {std::string(ExtraConfig::kMemoryReclaimThreadsHwMultiplier),
          folly::to<std::string>(memoryReclaimThreadsHwMultiplier)},
-        {std::string(ExtraConfig::kMemoryReclaimMaxWaitTime),
+        {std::string(ExtraConfig::kMaxMemoryArbitrationTime),
          folly::to<std::string>(arbitrationTimeoutNs) + "ns"},
         {std::string(ExtraConfig::kGlobalArbitrationEnabled),
          folly::to<std::string>(globalArtbitrationEnabled)},
@@ -576,7 +576,7 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) {
       SharedArbitrator::ExtraConfig::memoryPoolInitialCapacity(emptyConfigs),
       256 << 20);
   ASSERT_EQ(
-      SharedArbitrator::ExtraConfig::memoryReclaimMaxWaitTimeNs(emptyConfigs),
+      SharedArbitrator::ExtraConfig::maxMemoryArbitrationTimeNs(emptyConfigs),
       300'000'000'000UL);
   ASSERT_EQ(
       SharedArbitrator::ExtraConfig::globalArbitrationEnabled(emptyConfigs),
@@ -616,7 +616,7 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) {
   configs[std::string(
       SharedArbitrator::ExtraConfig::kMemoryPoolReservedCapacity)] = "200B";
   configs[std::string(
-      SharedArbitrator::ExtraConfig::kMemoryReclaimMaxWaitTime)] = "5000ms";
+      SharedArbitrator::ExtraConfig::kMaxMemoryArbitrationTime)] = "5000ms";
   configs[std::string(
       SharedArbitrator::ExtraConfig::kGlobalArbitrationEnabled)] = "true";
   configs[std::string(SharedArbitrator::ExtraConfig::kCheckUsageLeak)] =
@@ -643,8 +643,8 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) {
   ASSERT_EQ(
       SharedArbitrator::ExtraConfig::memoryPoolReservedCapacity(configs), 200);
   ASSERT_EQ(
-      SharedArbitrator::ExtraConfig::memoryReclaimMaxWaitTimeNs(configs),
-      5'000'000'000);
+      SharedArbitrator::ExtraConfig::maxMemoryArbitrationTimeNs(configs),
+      5'000'000'000UL);
   ASSERT_TRUE(SharedArbitrator::ExtraConfig::globalArbitrationEnabled(configs));
   ASSERT_FALSE(SharedArbitrator::ExtraConfig::checkUsageLeak(configs));
   ASSERT_EQ(
@@ -674,7 +674,7 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) {
   configs[std::string(
       SharedArbitrator::ExtraConfig::kMemoryPoolReservedCapacity)] = "invalid";
   configs[std::string(
-      SharedArbitrator::ExtraConfig::kMemoryReclaimMaxWaitTime)] = "invalid";
+      SharedArbitrator::ExtraConfig::kMaxMemoryArbitrationTime)] = "invalid";
   configs[std::string(
       SharedArbitrator::ExtraConfig::kGlobalArbitrationEnabled)] = "invalid";
   configs[std::string(SharedArbitrator::ExtraConfig::kCheckUsageLeak)] =
@@ -707,7 +707,7 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) {
       SharedArbitrator::ExtraConfig::memoryPoolReservedCapacity(configs),
       "Invalid capacity string 'invalid'");
   VELOX_ASSERT_THROW(
-      SharedArbitrator::ExtraConfig::memoryReclaimMaxWaitTimeNs(configs),
+      SharedArbitrator::ExtraConfig::maxMemoryArbitrationTimeNs(configs),
       "Invalid duration 'invalid'");
   VELOX_ASSERT_THROW(
       SharedArbitrator::ExtraConfig::globalArbitrationEnabled(configs),