[PR #748] [MERGED] [Store] Refine Complicated Constructor Parameters #1791

New issue

Closed

opened 2026-10-29 21:30:23 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 21:30:23 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/Mooncake/pull/748
Author: @ykwd
Created: 8/15/2025
Status: ✅ Merged
Merged: 8/18/2025
Merged by: @ykwd

Base: main ← Head: dev/store-config-refine

📝 Commits (10+)

c17273e Refactor configs
769cb28 Add validation logic
5752dd6 Refactor codes
1128b0c Add MasterServiceConfigBuilder
a6b040d chores
1ebecf1 Format code
96f3ee5 Use consistent variable name
94ae19c Fix unit tests
c539286 Minor fix
cbd477c Format code

📊 Changes

13 files changed (+705 additions, -270 deletions)

View changed files

➖ mooncake-store/include/config.h (+0 -33)
➕ mooncake-store/include/config_helper.h (+98 -0)
📝 mooncake-store/include/ha_helper.h (+3 -49)
➕ mooncake-store/include/master_config.h (+377 -0)
📝 mooncake-store/include/master_service.h (+3 -16)
📝 mooncake-store/include/rpc_service.h (+2 -15)
📝 mooncake-store/src/ha_helper.cpp (+14 -42)
📝 mooncake-store/src/master.cpp (+5 -18)
📝 mooncake-store/src/master_service.cpp (+16 -21)
📝 mooncake-store/src/rpc_service.cpp (+6 -16)
📝 mooncake-store/tests/master_metrics_test.cpp (+10 -6)
📝 mooncake-store/tests/master_service_ssd_test.cpp (+1 -11)
📝 mooncake-store/tests/master_service_test.cpp (+170 -43)

📄 Description

For classes like WrappedMasterService, MasterServiceSupervisor, and MasterService, the number of constructor parameters has gradually grown to a dozen or more. This makes adding new parameters or modifying existing ones increasingly cumbersome. It also becomes difficult to maintain consistency across the codebase—especially when some parameters have default values while others do not.

For example, in master_metrics_test.cpp, the following code became outdated without notice when the third parameter was replaced with another int parameter (the original one is enable_metric_reporting):

WrappedMasterService service_(false, default_kv_lease_ttl, true);

This PR addresses these issues by introducing a dedicated config class for each of the three classes mentioned above. All required parameters are defined within the config class, allowing them to be set by name rather than by position. It also clearly distinguishes between parameters with and without default values. For the latter, the config class enforces that a value must be explicitly set before use. Additionally, with named parameter settings, it also becomes easier to set a subset of parameters with non-default values.

With this PR, the following code can be simplified and well-maintained:

    // Current code in master_service_ssd_test.cpp
    return std::make_unique<MasterService>(
        false,                         // enable_gc
        DEFAULT_DEFAULT_KV_LEASE_TTL,  // default_kv_lease_ttl
        DEFAULT_KV_SOFT_PIN_TTL_MS,    // default_kv_soft_pin_ttl
        DEFAULT_ALLOW_EVICT_SOFT_PINNED_OBJECTS, DEFAULT_EVICTION_RATIO,
        DEFAULT_EVICTION_HIGH_WATERMARK_RATIO,
        0,  // view_version
        DEFAULT_CLIENT_LIVE_TTL_SEC,
        false,  // enable_ha
        DEFAULT_CLUSTER_ID, root_fs_dir, BufferAllocatorType::CACHELIB);

    // With this PR
    return std::make_unique<MasterService>(
        MasterServiceConfig::builder().set_root_fs_dir(root_fs_dir).build());

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/kvcache-ai/Mooncake/pull/748 **Author:** [@ykwd](https://github.com/ykwd) **Created:** 8/15/2025 **Status:** ✅ Merged **Merged:** 8/18/2025 **Merged by:** [@ykwd](https://github.com/ykwd) **Base:** `main` ← **Head:** `dev/store-config-refine` --- ### 📝 Commits (10+) - [`c17273e`](https://github.com/kvcache-ai/Mooncake/commit/c17273e999146c4cd41a5709b43b39236d60574b) Refactor configs - [`769cb28`](https://github.com/kvcache-ai/Mooncake/commit/769cb28b043fbc9f4d683aa4fd288d771b400c19) Add validation logic - [`5752dd6`](https://github.com/kvcache-ai/Mooncake/commit/5752dd659900d00b005dc24d767277e4241c16dd) Refactor codes - [`1128b0c`](https://github.com/kvcache-ai/Mooncake/commit/1128b0cf623f98f37ebb04a292e03f94881f7769) Add MasterServiceConfigBuilder - [`a6b040d`](https://github.com/kvcache-ai/Mooncake/commit/a6b040dbcc9d0e7cbc2e15563a385c20472ac160) chores - [`1ebecf1`](https://github.com/kvcache-ai/Mooncake/commit/1ebecf170dbadd0d9438d024bd92c9e4ed91d2cc) Format code - [`96f3ee5`](https://github.com/kvcache-ai/Mooncake/commit/96f3ee5a829bf9a1192797f0f4107ee8e77dcaac) Use consistent variable name - [`94ae19c`](https://github.com/kvcache-ai/Mooncake/commit/94ae19c0097822b91705424e0309754e6b494f0c) Fix unit tests - [`c539286`](https://github.com/kvcache-ai/Mooncake/commit/c539286729e8eda0cbb5776247e07b26b2450655) Minor fix - [`cbd477c`](https://github.com/kvcache-ai/Mooncake/commit/cbd477c627a8afa1649971aafe5c542e2f6e58b0) Format code ### 📊 Changes **13 files changed** (+705 additions, -270 deletions) <details> <summary>View changed files</summary> ➖ `mooncake-store/include/config.h` (+0 -33) ➕ `mooncake-store/include/config_helper.h` (+98 -0) 📝 `mooncake-store/include/ha_helper.h` (+3 -49) ➕ `mooncake-store/include/master_config.h` (+377 -0) 📝 `mooncake-store/include/master_service.h` (+3 -16) 📝 `mooncake-store/include/rpc_service.h` (+2 -15) 📝 `mooncake-store/src/ha_helper.cpp` (+14 -42) 📝 `mooncake-store/src/master.cpp` (+5 -18) 📝 `mooncake-store/src/master_service.cpp` (+16 -21) 📝 `mooncake-store/src/rpc_service.cpp` (+6 -16) 📝 `mooncake-store/tests/master_metrics_test.cpp` (+10 -6) 📝 `mooncake-store/tests/master_service_ssd_test.cpp` (+1 -11) 📝 `mooncake-store/tests/master_service_test.cpp` (+170 -43) </details> ### 📄 Description For classes like `WrappedMasterService`, `MasterServiceSupervisor`, and `MasterService`, the number of constructor parameters has gradually grown to a dozen or more. This makes adding new parameters or modifying existing ones increasingly cumbersome. It also becomes difficult to maintain consistency across the codebase—especially when some parameters have default values while others do not. For example, in `master_metrics_test.cpp`, the following code became outdated without notice when the third parameter was replaced with another `int` parameter (the original one is enable_metric_reporting): ```cpp WrappedMasterService service_(false, default_kv_lease_ttl, true); ``` This PR addresses these issues by introducing a dedicated config class for each of the three classes mentioned above. All required parameters are defined within the config class, allowing them to be set by name rather than by position. It also clearly distinguishes between parameters with and without default values. For the latter, the config class enforces that a value must be explicitly set before use. Additionally, with named parameter settings, it also becomes easier to set a subset of parameters with non-default values. With this PR, the following code can be simplified and well-maintained: ```c++ // Current code in master_service_ssd_test.cpp return std::make_unique<MasterService>( false, // enable_gc DEFAULT_DEFAULT_KV_LEASE_TTL, // default_kv_lease_ttl DEFAULT_KV_SOFT_PIN_TTL_MS, // default_kv_soft_pin_ttl DEFAULT_ALLOW_EVICT_SOFT_PINNED_OBJECTS, DEFAULT_EVICTION_RATIO, DEFAULT_EVICTION_HIGH_WATERMARK_RATIO, 0, // view_version DEFAULT_CLIENT_LIVE_TTL_SEC, false, // enable_ha DEFAULT_CLUSTER_ID, root_fs_dir, BufferAllocatorType::CACHELIB); ``` ```c++ // With this PR return std::make_unique<MasterService>( MasterServiceConfig::builder().set_root_fs_dir(root_fs_dir).build()); ``` --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-29 21:30:23 +00:00