From 47add822ab010685a64c985bf92fabb34e7f96e6 Mon Sep 17 00:00:00 2001 From: yuyang Date: Tue, 4 Nov 2025 17:25:15 +0800 Subject: [PATCH 01/13] add C++ builder for client --- mooncake-store/include/client.h | 30 +++++++++++ mooncake-store/include/pybind_client.h | 1 + mooncake-store/src/client.cpp | 69 ++++++++++++++++++++++++++ mooncake-store/src/pybind_client.cpp | 17 +++++-- 4 files changed, 114 insertions(+), 3 deletions(-) diff --git a/mooncake-store/include/client.h b/mooncake-store/include/client.h index 725c890e5..3ccfa6542 100644 --- a/mooncake-store/include/client.h +++ b/mooncake-store/include/client.h @@ -364,4 +364,34 @@ class Client { UUID client_id_; }; +/** + * @brief Fluent builder for configuring a mooncake::Client instance. + * + * Provides readable, type-safe setters with sensible defaults so callers can + * specify only the options they need while reusing existing Client::Create + * logic under the hood. + */ +class MooncakeStoreBuilder { + public: + MooncakeStoreBuilder& WithLocalHostname(std::string local_hostname); + MooncakeStoreBuilder& WithMetadataConnectionString( + std::string metadata_connstring); + MooncakeStoreBuilder& UsingProtocol(std::string protocol); + MooncakeStoreBuilder& WithRdmaDeviceNames(std::string device_names); + MooncakeStoreBuilder& WithMasterServerEntry( + std::string master_server_entry); + MooncakeStoreBuilder& WithExistingTransferEngine( + std::shared_ptr transfer_engine); + + [[nodiscard]] std::optional> Build(); + + private: + std::optional local_hostname_; + std::optional metadata_connstring_; + std::string protocol_ = "tcp"; + std::optional device_names_; + std::string master_server_entry_ = kDefaultMasterAddress; + std::shared_ptr transfer_engine_ = nullptr; +}; + } // namespace mooncake diff --git a/mooncake-store/include/pybind_client.h b/mooncake-store/include/pybind_client.h index d79c0aab0..b810d2f4e 100644 --- a/mooncake-store/include/pybind_client.h +++ b/mooncake-store/include/pybind_client.h @@ -82,6 +82,7 @@ class PyClient { // Factory to create shared instances and auto-register to ResourceTracker static std::shared_ptr create(); + [[deprecated("Use MooncakeStoreBuilder instead")]] int setup(const std::string &local_hostname, const std::string &metadata_server, size_t global_segment_size = 1024 * 1024 * 16, diff --git a/mooncake-store/src/client.cpp b/mooncake-store/src/client.cpp index cf5ccbabf..656edf34e 100644 --- a/mooncake-store/src/client.cpp +++ b/mooncake-store/src/client.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -406,6 +407,74 @@ std::optional> Client::Create( return client; } +MooncakeStoreBuilder& MooncakeStoreBuilder::WithLocalHostname( + std::string local_hostname) { + local_hostname_ = std::move(local_hostname); + return *this; +} + +MooncakeStoreBuilder& MooncakeStoreBuilder::WithMetadataConnectionString( + std::string metadata_connstring) { + metadata_connstring_ = std::move(metadata_connstring); + return *this; +} + +MooncakeStoreBuilder& MooncakeStoreBuilder::UsingProtocol( + std::string protocol) { + protocol_ = std::move(protocol); + return *this; +} + +MooncakeStoreBuilder& MooncakeStoreBuilder::WithRdmaDeviceNames( + std::string device_names) { + if (device_names.empty()) { + device_names_.reset(); + } else { + device_names_ = std::move(device_names); + } + return *this; +} + +MooncakeStoreBuilder& MooncakeStoreBuilder::WithMasterServerEntry( + std::string master_server_entry) { + master_server_entry_ = std::move(master_server_entry); + return *this; +} + +MooncakeStoreBuilder& MooncakeStoreBuilder::WithExistingTransferEngine( + std::shared_ptr transfer_engine) { + transfer_engine_ = std::move(transfer_engine); + return *this; +} + +std::optional> MooncakeStoreBuilder::Build() { + std::vector missing; + if (!local_hostname_) { + missing.emplace_back("local_hostname"); + } + if (!metadata_connstring_) { + missing.emplace_back("metadata_connstring"); + } + + if (!missing.empty()) { + std::string joined; + joined.reserve(missing.size() * 16); + for (size_t i = 0; i < missing.size(); ++i) { + if (i != 0) { + joined.append(", "); + } + joined.append(missing[i]); + } + LOG(ERROR) << "MooncakeStoreBuilder missing required fields: " + << joined; + return std::nullopt; + } + + return Client::Create(*local_hostname_, *metadata_connstring_, protocol_, + device_names_, master_server_entry_, + transfer_engine_); +} + tl::expected Client::Get(const std::string& object_key, std::vector& slices) { auto query_result = Query(object_key); diff --git a/mooncake-store/src/pybind_client.cpp b/mooncake-store/src/pybind_client.cpp index 996d4aa75..b7a1fb6b2 100644 --- a/mooncake-store/src/pybind_client.cpp +++ b/mooncake-store/src/pybind_client.cpp @@ -178,9 +178,20 @@ tl::expected PyClient::setup_internal( (rdma_devices.empty() ? std::nullopt : std::make_optional(rdma_devices)); - auto client_opt = mooncake::Client::Create( - this->local_hostname, metadata_server, protocol, device_name, - master_server_addr, transfer_engine); + MooncakeStoreBuilder builder; + builder.WithLocalHostname(this->local_hostname) + .WithMetadataConnectionString(metadata_server) + .UsingProtocol(protocol) + .WithMasterServerEntry(master_server_addr); + + if (device_name) { + builder.WithRdmaDeviceNames(*device_name); + } + if (transfer_engine) { + builder.WithExistingTransferEngine(transfer_engine); + } + + auto client_opt = builder.Build(); if (!client_opt) { LOG(ERROR) << "Failed to create client"; return tl::unexpected(ErrorCode::INVALID_PARAMS); From 58650f84503b7d6e6ec535e5fcd4f978530f9c59 Mon Sep 17 00:00:00 2001 From: yuyang Date: Tue, 4 Nov 2025 17:42:14 +0800 Subject: [PATCH 02/13] changed builder to const --- mooncake-store/include/client.h | 2 +- mooncake-store/src/client.cpp | 8 ++++++-- mooncake-store/src/pybind_client.cpp | 9 ++++++++- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/mooncake-store/include/client.h b/mooncake-store/include/client.h index 3ccfa6542..404033119 100644 --- a/mooncake-store/include/client.h +++ b/mooncake-store/include/client.h @@ -383,7 +383,7 @@ class MooncakeStoreBuilder { MooncakeStoreBuilder& WithExistingTransferEngine( std::shared_ptr transfer_engine); - [[nodiscard]] std::optional> Build(); + [[nodiscard]] std::optional> Build() const; private: std::optional local_hostname_; diff --git a/mooncake-store/src/client.cpp b/mooncake-store/src/client.cpp index 656edf34e..3f1dcd7ee 100644 --- a/mooncake-store/src/client.cpp +++ b/mooncake-store/src/client.cpp @@ -447,7 +447,7 @@ MooncakeStoreBuilder& MooncakeStoreBuilder::WithExistingTransferEngine( return *this; } -std::optional> MooncakeStoreBuilder::Build() { +std::optional> MooncakeStoreBuilder::Build() const { std::vector missing; if (!local_hostname_) { missing.emplace_back("local_hostname"); @@ -463,7 +463,11 @@ std::optional> MooncakeStoreBuilder::Build() { if (i != 0) { joined.append(", "); } - joined.append(missing[i]); + joined.append(missing[0]); + for (size_t i = 1; i < missing.size(); ++i) { + joined.append(", "); + joined.append(missing[i]); + } } LOG(ERROR) << "MooncakeStoreBuilder missing required fields: " << joined; diff --git a/mooncake-store/src/pybind_client.cpp b/mooncake-store/src/pybind_client.cpp index b7a1fb6b2..491c558c4 100644 --- a/mooncake-store/src/pybind_client.cpp +++ b/mooncake-store/src/pybind_client.cpp @@ -191,7 +191,14 @@ tl::expected PyClient::setup_internal( builder.WithExistingTransferEngine(transfer_engine); } - auto client_opt = builder.Build(); + auto client_opt = MooncakeStoreBuilder() + .WithLocalHostname(this->local_hostname) + .WithMetadataConnectionString(metadata_server) + .UsingProtocol(protocol) + .WithMasterServerEntry(master_server_addr) + .WithRdmaDeviceNames(device_name.value_or("")) + .WithExistingTransferEngine(transfer_engine) + .Build(); if (!client_opt) { LOG(ERROR) << "Failed to create client"; return tl::unexpected(ErrorCode::INVALID_PARAMS); From 4fbe66acad2734fb46630e84a363c4bd26cf91cd Mon Sep 17 00:00:00 2001 From: yuyang Date: Tue, 4 Nov 2025 17:50:02 +0800 Subject: [PATCH 03/13] reformat the code --- mooncake-store/include/pybind_client.h | 22 +++++++++++----------- mooncake-store/src/pybind_client.cpp | 14 +++++++------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/mooncake-store/include/pybind_client.h b/mooncake-store/include/pybind_client.h index b810d2f4e..29d336b6e 100644 --- a/mooncake-store/include/pybind_client.h +++ b/mooncake-store/include/pybind_client.h @@ -21,8 +21,9 @@ constexpr bool is_supported_return_type_v = std::is_void_v || std::is_integral_v; template - requires is_supported_return_type_v -int64_t to_py_ret(const tl::expected &exp) noexcept { +requires is_supported_return_type_v int64_t +to_py_ret(const tl::expected &exp) +noexcept { if (!exp) { return static_cast(toInt(exp.error())); } @@ -82,15 +83,14 @@ class PyClient { // Factory to create shared instances and auto-register to ResourceTracker static std::shared_ptr create(); - [[deprecated("Use MooncakeStoreBuilder instead")]] - int setup(const std::string &local_hostname, - const std::string &metadata_server, - size_t global_segment_size = 1024 * 1024 * 16, - size_t local_buffer_size = 1024 * 1024 * 16, - const std::string &protocol = "tcp", - const std::string &rdma_devices = "", - const std::string &master_server_addr = "127.0.0.1:50051", - const std::shared_ptr &transfer_engine = nullptr); + [[deprecated("Use MooncakeStoreBuilder instead")]] int setup( + const std::string &local_hostname, const std::string &metadata_server, + size_t global_segment_size = 1024 * 1024 * 16, + size_t local_buffer_size = 1024 * 1024 * 16, + const std::string &protocol = "tcp", + const std::string &rdma_devices = "", + const std::string &master_server_addr = "127.0.0.1:50051", + const std::shared_ptr &transfer_engine = nullptr); int initAll(const std::string &protocol, const std::string &device_name, size_t mount_segment_size = 1024 * 1024 * 16); // Default 16MB diff --git a/mooncake-store/src/pybind_client.cpp b/mooncake-store/src/pybind_client.cpp index 491c558c4..93ba613b3 100644 --- a/mooncake-store/src/pybind_client.cpp +++ b/mooncake-store/src/pybind_client.cpp @@ -192,13 +192,13 @@ tl::expected PyClient::setup_internal( } auto client_opt = MooncakeStoreBuilder() - .WithLocalHostname(this->local_hostname) - .WithMetadataConnectionString(metadata_server) - .UsingProtocol(protocol) - .WithMasterServerEntry(master_server_addr) - .WithRdmaDeviceNames(device_name.value_or("")) - .WithExistingTransferEngine(transfer_engine) - .Build(); + .WithLocalHostname(this->local_hostname) + .WithMetadataConnectionString(metadata_server) + .UsingProtocol(protocol) + .WithMasterServerEntry(master_server_addr) + .WithRdmaDeviceNames(device_name.value_or("")) + .WithExistingTransferEngine(transfer_engine) + .Build(); if (!client_opt) { LOG(ERROR) << "Failed to create client"; return tl::unexpected(ErrorCode::INVALID_PARAMS); From 2fb2b7e4c7efb5fa5140ed2e3b76c65a188cd38a Mon Sep 17 00:00:00 2001 From: yuyang Date: Wed, 5 Nov 2025 15:56:36 +0800 Subject: [PATCH 04/13] 1. fixed names and return value types 2. added tests for the builder --- mooncake-store/include/client.h | 9 ++-- mooncake-store/src/client.cpp | 35 +++++++------- mooncake-store/src/pybind_client.cpp | 12 ++--- mooncake-store/tests/CMakeLists.txt | 1 + mooncake-store/tests/builder_test.cpp | 70 +++++++++++++++++++++++++++ 5 files changed, 100 insertions(+), 27 deletions(-) create mode 100644 mooncake-store/tests/builder_test.cpp diff --git a/mooncake-store/include/client.h b/mooncake-store/include/client.h index 404033119..156812345 100644 --- a/mooncake-store/include/client.h +++ b/mooncake-store/include/client.h @@ -364,6 +364,7 @@ class Client { UUID client_id_; }; + /** * @brief Fluent builder for configuring a mooncake::Client instance. * @@ -376,14 +377,14 @@ class MooncakeStoreBuilder { MooncakeStoreBuilder& WithLocalHostname(std::string local_hostname); MooncakeStoreBuilder& WithMetadataConnectionString( std::string metadata_connstring); - MooncakeStoreBuilder& UsingProtocol(std::string protocol); - MooncakeStoreBuilder& WithRdmaDeviceNames(std::string device_names); - MooncakeStoreBuilder& WithMasterServerEntry( + MooncakeStoreBuilder& WithProtocol(std::string protocol); + MooncakeStoreBuilder& WithTransferEngineArgs(std::string engine_args); + MooncakeStoreBuilder& WithMasterEndpoint( std::string master_server_entry); MooncakeStoreBuilder& WithExistingTransferEngine( std::shared_ptr transfer_engine); - [[nodiscard]] std::optional> Build() const; + [[nodiscard]] tl::expected, std::string> Build() const; private: std::optional local_hostname_; diff --git a/mooncake-store/src/client.cpp b/mooncake-store/src/client.cpp index 3f1dcd7ee..4a79cca82 100644 --- a/mooncake-store/src/client.cpp +++ b/mooncake-store/src/client.cpp @@ -419,23 +419,20 @@ MooncakeStoreBuilder& MooncakeStoreBuilder::WithMetadataConnectionString( return *this; } -MooncakeStoreBuilder& MooncakeStoreBuilder::UsingProtocol( +MooncakeStoreBuilder& MooncakeStoreBuilder::WithProtocol( std::string protocol) { protocol_ = std::move(protocol); return *this; } -MooncakeStoreBuilder& MooncakeStoreBuilder::WithRdmaDeviceNames( - std::string device_names) { - if (device_names.empty()) { - device_names_.reset(); - } else { - device_names_ = std::move(device_names); - } +MooncakeStoreBuilder& MooncakeStoreBuilder::WithTransferEngineArgs( + std::string engine_args) { + // Can add some other engine arguments + device_names_ = std::move(engine_args); return *this; } -MooncakeStoreBuilder& MooncakeStoreBuilder::WithMasterServerEntry( +MooncakeStoreBuilder& MooncakeStoreBuilder::WithMasterEndpoint( std::string master_server_entry) { master_server_entry_ = std::move(master_server_entry); return *this; @@ -447,7 +444,7 @@ MooncakeStoreBuilder& MooncakeStoreBuilder::WithExistingTransferEngine( return *this; } -std::optional> MooncakeStoreBuilder::Build() const { +tl::expected, std::string> MooncakeStoreBuilder::Build() const { std::vector missing; if (!local_hostname_) { missing.emplace_back("local_hostname"); @@ -469,14 +466,18 @@ std::optional> MooncakeStoreBuilder::Build() const { joined.append(missing[i]); } } - LOG(ERROR) << "MooncakeStoreBuilder missing required fields: " - << joined; - return std::nullopt; + auto error_msg = "MooncakeStoreBuilder missing required fields: " + joined; + LOG(ERROR) << error_msg; + return tl::make_unexpected(std::move(error_msg)); } - - return Client::Create(*local_hostname_, *metadata_connstring_, protocol_, - device_names_, master_server_entry_, - transfer_engine_); + auto client = Client::Create(*local_hostname_, *metadata_connstring_, + protocol_, device_names_, master_server_entry_, + transfer_engine_); + if (!client) { + std::string error_msg = "Client creation failed"; + return tl::make_unexpected(std::move(error_msg)); + } + return *client; } tl::expected Client::Get(const std::string& object_key, diff --git a/mooncake-store/src/pybind_client.cpp b/mooncake-store/src/pybind_client.cpp index 93ba613b3..210fa15c5 100644 --- a/mooncake-store/src/pybind_client.cpp +++ b/mooncake-store/src/pybind_client.cpp @@ -181,11 +181,11 @@ tl::expected PyClient::setup_internal( MooncakeStoreBuilder builder; builder.WithLocalHostname(this->local_hostname) .WithMetadataConnectionString(metadata_server) - .UsingProtocol(protocol) - .WithMasterServerEntry(master_server_addr); + .WithProtocol(protocol) + .WithMasterEndpoint(master_server_addr); if (device_name) { - builder.WithRdmaDeviceNames(*device_name); + builder.WithTransferEngineArgs(*device_name); } if (transfer_engine) { builder.WithExistingTransferEngine(transfer_engine); @@ -194,9 +194,9 @@ tl::expected PyClient::setup_internal( auto client_opt = MooncakeStoreBuilder() .WithLocalHostname(this->local_hostname) .WithMetadataConnectionString(metadata_server) - .UsingProtocol(protocol) - .WithMasterServerEntry(master_server_addr) - .WithRdmaDeviceNames(device_name.value_or("")) + .WithProtocol(protocol) + .WithMasterEndpoint(master_server_addr) + .WithTransferEngineArgs(device_name.value_or("")) .WithExistingTransferEngine(transfer_engine) .Build(); if (!client_opt) { diff --git a/mooncake-store/tests/CMakeLists.txt b/mooncake-store/tests/CMakeLists.txt index 28bd63c97..732e8b345 100644 --- a/mooncake-store/tests/CMakeLists.txt +++ b/mooncake-store/tests/CMakeLists.txt @@ -32,6 +32,7 @@ add_store_test(serializer_test serializer_test.cpp) add_store_test(non_ha_reconnect_test non_ha_reconnect_test.cpp) add_store_test(storage_backend_test storage_backend_test.cpp) add_store_test(mutex_test mutex_test.cpp) +add_store_test(builder_test builder_test.cpp) add_subdirectory(e2e) add_executable(high_availability_test high_availability_test.cpp) diff --git a/mooncake-store/tests/builder_test.cpp b/mooncake-store/tests/builder_test.cpp new file mode 100644 index 000000000..27bca74bb --- /dev/null +++ b/mooncake-store/tests/builder_test.cpp @@ -0,0 +1,70 @@ +#include +#include + +#include "client.h" + +namespace mooncake::test { + +class MooncakeStoreBuilderTest : public ::testing::Test { + protected: + void SetUp() override { + google::InitGoogleLogging("MooncakeStoreBuilderTest"); + FLAGS_logtostderr = true; + } + + void TearDown() override { google::ShutdownGoogleLogging(); } +}; + +TEST_F(MooncakeStoreBuilderTest, MissingAllRequiredFields) { + MooncakeStoreBuilder builder; + + auto result = builder.Build(); + + ASSERT_FALSE(result.has_value()); + const std::string& error = result.error(); + EXPECT_NE(error.find("local_hostname"), std::string::npos); + EXPECT_NE(error.find("metadata_connstring"), std::string::npos); +} + +TEST_F(MooncakeStoreBuilderTest, MissingLocalHostnameOnly) { + MooncakeStoreBuilder builder; + builder.WithMetadataConnectionString("metastore:1234"); + + auto result = builder.Build(); + + ASSERT_FALSE(result.has_value()); + const std::string& error = result.error(); + EXPECT_NE(error.find("local_hostname"), std::string::npos); + EXPECT_EQ(error.find("metadata_connstring"), std::string::npos); +} + +TEST_F(MooncakeStoreBuilderTest, MissingMetadataConnectionStringOnly) { + MooncakeStoreBuilder builder; + builder.WithLocalHostname("localhost:1234"); + + auto result = builder.Build(); + + ASSERT_FALSE(result.has_value()); + const std::string& error = result.error(); + EXPECT_NE(error.find("metadata_connstring"), std::string::npos); + EXPECT_EQ(error.find("local_hostname"), std::string::npos); +} + +TEST_F(MooncakeStoreBuilderTest, ProvidedAllRequiredFields) { + MooncakeStoreBuilder builder; + builder.WithLocalHostname("localhost:1234"); + builder.WithMetadataConnectionString("metadata:5678"); + + auto result = builder.Build(); + + if (result.has_value()) { + SUCCEED(); + } else { + const std::string& error = result.error(); + EXPECT_EQ(error.find("missing required fields"), std::string::npos); + EXPECT_EQ(error.find("local_hostname"), std::string::npos); + EXPECT_EQ(error.find("metadata_connstring"), std::string::npos); + } +} + +} // namespace mooncake::test From a9aa54c70ec33bf44bef3a808313f81a12928206 Mon Sep 17 00:00:00 2001 From: yuyang Date: Wed, 5 Nov 2025 15:59:16 +0800 Subject: [PATCH 05/13] reformat the codes --- mooncake-store/include/client.h | 7 +++---- mooncake-store/src/client.cpp | 15 ++++++++------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/mooncake-store/include/client.h b/mooncake-store/include/client.h index 156812345..44d6dca61 100644 --- a/mooncake-store/include/client.h +++ b/mooncake-store/include/client.h @@ -364,7 +364,6 @@ class Client { UUID client_id_; }; - /** * @brief Fluent builder for configuring a mooncake::Client instance. * @@ -379,12 +378,12 @@ class MooncakeStoreBuilder { std::string metadata_connstring); MooncakeStoreBuilder& WithProtocol(std::string protocol); MooncakeStoreBuilder& WithTransferEngineArgs(std::string engine_args); - MooncakeStoreBuilder& WithMasterEndpoint( - std::string master_server_entry); + MooncakeStoreBuilder& WithMasterEndpoint(std::string master_server_entry); MooncakeStoreBuilder& WithExistingTransferEngine( std::shared_ptr transfer_engine); - [[nodiscard]] tl::expected, std::string> Build() const; + [[nodiscard]] tl::expected, std::string> Build() + const; private: std::optional local_hostname_; diff --git a/mooncake-store/src/client.cpp b/mooncake-store/src/client.cpp index 4a79cca82..5f48ded59 100644 --- a/mooncake-store/src/client.cpp +++ b/mooncake-store/src/client.cpp @@ -419,8 +419,7 @@ MooncakeStoreBuilder& MooncakeStoreBuilder::WithMetadataConnectionString( return *this; } -MooncakeStoreBuilder& MooncakeStoreBuilder::WithProtocol( - std::string protocol) { +MooncakeStoreBuilder& MooncakeStoreBuilder::WithProtocol(std::string protocol) { protocol_ = std::move(protocol); return *this; } @@ -444,7 +443,8 @@ MooncakeStoreBuilder& MooncakeStoreBuilder::WithExistingTransferEngine( return *this; } -tl::expected, std::string> MooncakeStoreBuilder::Build() const { +tl::expected, std::string> MooncakeStoreBuilder::Build() + const { std::vector missing; if (!local_hostname_) { missing.emplace_back("local_hostname"); @@ -466,13 +466,14 @@ tl::expected, std::string> MooncakeStoreBuilder::Build() joined.append(missing[i]); } } - auto error_msg = "MooncakeStoreBuilder missing required fields: " + joined; + auto error_msg = + "MooncakeStoreBuilder missing required fields: " + joined; LOG(ERROR) << error_msg; return tl::make_unexpected(std::move(error_msg)); } - auto client = Client::Create(*local_hostname_, *metadata_connstring_, - protocol_, device_names_, master_server_entry_, - transfer_engine_); + auto client = + Client::Create(*local_hostname_, *metadata_connstring_, protocol_, + device_names_, master_server_entry_, transfer_engine_); if (!client) { std::string error_msg = "Client creation failed"; return tl::make_unexpected(std::move(error_msg)); From 58766bcfdbbd6e647f1a9c18bfaed20975cd3e33 Mon Sep 17 00:00:00 2001 From: yuyang Date: Wed, 5 Nov 2025 17:17:34 +0800 Subject: [PATCH 06/13] style: clang-format-16 for pybind_client.h --- mooncake-store/include/pybind_client.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/mooncake-store/include/pybind_client.h b/mooncake-store/include/pybind_client.h index 29d336b6e..b919b13b8 100644 --- a/mooncake-store/include/pybind_client.h +++ b/mooncake-store/include/pybind_client.h @@ -21,9 +21,8 @@ constexpr bool is_supported_return_type_v = std::is_void_v || std::is_integral_v; template -requires is_supported_return_type_v int64_t -to_py_ret(const tl::expected &exp) -noexcept { + requires is_supported_return_type_v +int64_t to_py_ret(const tl::expected &exp) noexcept { if (!exp) { return static_cast(toInt(exp.error())); } From 8010e075d814f03adcbd5a6a445e916341ca1767 Mon Sep 17 00:00:00 2001 From: yuyang Date: Wed, 5 Nov 2025 17:49:34 +0800 Subject: [PATCH 07/13] revert create in setup_internal function --- mooncake-store/src/pybind_client.cpp | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/mooncake-store/src/pybind_client.cpp b/mooncake-store/src/pybind_client.cpp index 210fa15c5..93e4eb6a3 100644 --- a/mooncake-store/src/pybind_client.cpp +++ b/mooncake-store/src/pybind_client.cpp @@ -177,28 +177,9 @@ tl::expected PyClient::setup_internal( std::optional device_name = (rdma_devices.empty() ? std::nullopt : std::make_optional(rdma_devices)); - - MooncakeStoreBuilder builder; - builder.WithLocalHostname(this->local_hostname) - .WithMetadataConnectionString(metadata_server) - .WithProtocol(protocol) - .WithMasterEndpoint(master_server_addr); - - if (device_name) { - builder.WithTransferEngineArgs(*device_name); - } - if (transfer_engine) { - builder.WithExistingTransferEngine(transfer_engine); - } - - auto client_opt = MooncakeStoreBuilder() - .WithLocalHostname(this->local_hostname) - .WithMetadataConnectionString(metadata_server) - .WithProtocol(protocol) - .WithMasterEndpoint(master_server_addr) - .WithTransferEngineArgs(device_name.value_or("")) - .WithExistingTransferEngine(transfer_engine) - .Build(); + auto client_opt = mooncake::Client::Create( + this->local_hostname, metadata_server, protocol, device_name, + master_server_addr, transfer_engine); if (!client_opt) { LOG(ERROR) << "Failed to create client"; return tl::unexpected(ErrorCode::INVALID_PARAMS); From a6a7dc3725578ff105874a62912d548c838b1318 Mon Sep 17 00:00:00 2001 From: yuyang Date: Thu, 6 Nov 2025 16:06:27 +0800 Subject: [PATCH 08/13] 1. revert deprecated 2. integrate builder test to client builder tests. --- mooncake-store/include/pybind_client.h | 21 +++--- mooncake-store/tests/CMakeLists.txt | 1 - mooncake-store/tests/builder_test.cpp | 70 ------------------- .../tests/client_integration_test.cpp | 62 ++++++++++++++++ 4 files changed, 73 insertions(+), 81 deletions(-) delete mode 100644 mooncake-store/tests/builder_test.cpp diff --git a/mooncake-store/include/pybind_client.h b/mooncake-store/include/pybind_client.h index b919b13b8..31f0e5dd5 100644 --- a/mooncake-store/include/pybind_client.h +++ b/mooncake-store/include/pybind_client.h @@ -21,8 +21,9 @@ constexpr bool is_supported_return_type_v = std::is_void_v || std::is_integral_v; template - requires is_supported_return_type_v -int64_t to_py_ret(const tl::expected &exp) noexcept { +requires is_supported_return_type_v int64_t +to_py_ret(const tl::expected &exp) +noexcept { if (!exp) { return static_cast(toInt(exp.error())); } @@ -82,14 +83,14 @@ class PyClient { // Factory to create shared instances and auto-register to ResourceTracker static std::shared_ptr create(); - [[deprecated("Use MooncakeStoreBuilder instead")]] int setup( - const std::string &local_hostname, const std::string &metadata_server, - size_t global_segment_size = 1024 * 1024 * 16, - size_t local_buffer_size = 1024 * 1024 * 16, - const std::string &protocol = "tcp", - const std::string &rdma_devices = "", - const std::string &master_server_addr = "127.0.0.1:50051", - const std::shared_ptr &transfer_engine = nullptr); + int setup(const std::string &local_hostname, + const std::string &metadata_server, + size_t global_segment_size = 1024 * 1024 * 16, + size_t local_buffer_size = 1024 * 1024 * 16, + const std::string &protocol = "tcp", + const std::string &rdma_devices = "", + const std::string &master_server_addr = "127.0.0.1:50051", + const std::shared_ptr &transfer_engine = nullptr); int initAll(const std::string &protocol, const std::string &device_name, size_t mount_segment_size = 1024 * 1024 * 16); // Default 16MB diff --git a/mooncake-store/tests/CMakeLists.txt b/mooncake-store/tests/CMakeLists.txt index 732e8b345..28bd63c97 100644 --- a/mooncake-store/tests/CMakeLists.txt +++ b/mooncake-store/tests/CMakeLists.txt @@ -32,7 +32,6 @@ add_store_test(serializer_test serializer_test.cpp) add_store_test(non_ha_reconnect_test non_ha_reconnect_test.cpp) add_store_test(storage_backend_test storage_backend_test.cpp) add_store_test(mutex_test mutex_test.cpp) -add_store_test(builder_test builder_test.cpp) add_subdirectory(e2e) add_executable(high_availability_test high_availability_test.cpp) diff --git a/mooncake-store/tests/builder_test.cpp b/mooncake-store/tests/builder_test.cpp deleted file mode 100644 index 27bca74bb..000000000 --- a/mooncake-store/tests/builder_test.cpp +++ /dev/null @@ -1,70 +0,0 @@ -#include -#include - -#include "client.h" - -namespace mooncake::test { - -class MooncakeStoreBuilderTest : public ::testing::Test { - protected: - void SetUp() override { - google::InitGoogleLogging("MooncakeStoreBuilderTest"); - FLAGS_logtostderr = true; - } - - void TearDown() override { google::ShutdownGoogleLogging(); } -}; - -TEST_F(MooncakeStoreBuilderTest, MissingAllRequiredFields) { - MooncakeStoreBuilder builder; - - auto result = builder.Build(); - - ASSERT_FALSE(result.has_value()); - const std::string& error = result.error(); - EXPECT_NE(error.find("local_hostname"), std::string::npos); - EXPECT_NE(error.find("metadata_connstring"), std::string::npos); -} - -TEST_F(MooncakeStoreBuilderTest, MissingLocalHostnameOnly) { - MooncakeStoreBuilder builder; - builder.WithMetadataConnectionString("metastore:1234"); - - auto result = builder.Build(); - - ASSERT_FALSE(result.has_value()); - const std::string& error = result.error(); - EXPECT_NE(error.find("local_hostname"), std::string::npos); - EXPECT_EQ(error.find("metadata_connstring"), std::string::npos); -} - -TEST_F(MooncakeStoreBuilderTest, MissingMetadataConnectionStringOnly) { - MooncakeStoreBuilder builder; - builder.WithLocalHostname("localhost:1234"); - - auto result = builder.Build(); - - ASSERT_FALSE(result.has_value()); - const std::string& error = result.error(); - EXPECT_NE(error.find("metadata_connstring"), std::string::npos); - EXPECT_EQ(error.find("local_hostname"), std::string::npos); -} - -TEST_F(MooncakeStoreBuilderTest, ProvidedAllRequiredFields) { - MooncakeStoreBuilder builder; - builder.WithLocalHostname("localhost:1234"); - builder.WithMetadataConnectionString("metadata:5678"); - - auto result = builder.Build(); - - if (result.has_value()) { - SUCCEED(); - } else { - const std::string& error = result.error(); - EXPECT_EQ(error.find("missing required fields"), std::string::npos); - EXPECT_EQ(error.find("local_hostname"), std::string::npos); - EXPECT_EQ(error.find("metadata_connstring"), std::string::npos); - } -} - -} // namespace mooncake::test diff --git a/mooncake-store/tests/client_integration_test.cpp b/mooncake-store/tests/client_integration_test.cpp index e42d50a3b..15be768d1 100644 --- a/mooncake-store/tests/client_integration_test.cpp +++ b/mooncake-store/tests/client_integration_test.cpp @@ -22,6 +22,68 @@ DEFINE_uint64(default_kv_lease_ttl, mooncake::DEFAULT_DEFAULT_KV_LEASE_TTL, namespace mooncake { namespace testing { +class MooncakeStoreBuilderTest : public ::testing::Test { + protected: + void SetUp() override { + google::InitGoogleLogging("MooncakeStoreBuilderTest"); + FLAGS_logtostderr = true; + } + + void TearDown() override { google::ShutdownGoogleLogging(); } +}; + +TEST_F(MooncakeStoreBuilderTest, MissingAllRequiredFields) { + MooncakeStoreBuilder builder; + + auto result = builder.Build(); + + ASSERT_FALSE(result.has_value()); + const std::string& error = result.error(); + EXPECT_NE(error.find("local_hostname"), std::string::npos); + EXPECT_NE(error.find("metadata_connstring"), std::string::npos); +} + +TEST_F(MooncakeStoreBuilderTest, MissingLocalHostnameOnly) { + MooncakeStoreBuilder builder; + builder.WithMetadataConnectionString("metastore:1234"); + + auto result = builder.Build(); + + ASSERT_FALSE(result.has_value()); + const std::string& error = result.error(); + EXPECT_NE(error.find("local_hostname"), std::string::npos); + EXPECT_EQ(error.find("metadata_connstring"), std::string::npos); +} + +TEST_F(MooncakeStoreBuilderTest, MissingMetadataConnectionStringOnly) { + MooncakeStoreBuilder builder; + builder.WithLocalHostname("localhost:1234"); + + auto result = builder.Build(); + + ASSERT_FALSE(result.has_value()); + const std::string& error = result.error(); + EXPECT_NE(error.find("metadata_connstring"), std::string::npos); + EXPECT_EQ(error.find("local_hostname"), std::string::npos); +} + +TEST_F(MooncakeStoreBuilderTest, ProvidedAllRequiredFields) { + MooncakeStoreBuilder builder; + builder.WithLocalHostname("localhost:1234"); + builder.WithMetadataConnectionString("metadata:5678"); + + auto result = builder.Build(); + + if (result.has_value()) { + SUCCEED(); + } else { + const std::string& error = result.error(); + EXPECT_EQ(error.find("missing required fields"), std::string::npos); + EXPECT_EQ(error.find("local_hostname"), std::string::npos); + EXPECT_EQ(error.find("metadata_connstring"), std::string::npos); + } +} + class ClientIntegrationTest : public ::testing::Test { protected: static std::shared_ptr CreateClient(const std::string& host_name) { From ff674c545d1dc15599bba9f1adc5c42a288cb0ee Mon Sep 17 00:00:00 2001 From: yuyang Date: Fri, 7 Nov 2025 11:45:23 +0800 Subject: [PATCH 09/13] fixed naming bugs --- mooncake-store/include/client.h | 4 ++-- mooncake-store/src/client.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/mooncake-store/include/client.h b/mooncake-store/include/client.h index 44d6dca61..4578c688e 100644 --- a/mooncake-store/include/client.h +++ b/mooncake-store/include/client.h @@ -376,7 +376,7 @@ class MooncakeStoreBuilder { MooncakeStoreBuilder& WithLocalHostname(std::string local_hostname); MooncakeStoreBuilder& WithMetadataConnectionString( std::string metadata_connstring); - MooncakeStoreBuilder& WithProtocol(std::string protocol); + MooncakeStoreBuilder& WithProtocolArgs(std::string protocol); MooncakeStoreBuilder& WithTransferEngineArgs(std::string engine_args); MooncakeStoreBuilder& WithMasterEndpoint(std::string master_server_entry); MooncakeStoreBuilder& WithExistingTransferEngine( @@ -389,7 +389,7 @@ class MooncakeStoreBuilder { std::optional local_hostname_; std::optional metadata_connstring_; std::string protocol_ = "tcp"; - std::optional device_names_; + std::optional engine_args_; std::string master_server_entry_ = kDefaultMasterAddress; std::shared_ptr transfer_engine_ = nullptr; }; diff --git a/mooncake-store/src/client.cpp b/mooncake-store/src/client.cpp index 5f48ded59..821d5e19b 100644 --- a/mooncake-store/src/client.cpp +++ b/mooncake-store/src/client.cpp @@ -419,7 +419,7 @@ MooncakeStoreBuilder& MooncakeStoreBuilder::WithMetadataConnectionString( return *this; } -MooncakeStoreBuilder& MooncakeStoreBuilder::WithProtocol(std::string protocol) { +MooncakeStoreBuilder& MooncakeStoreBuilder::WithProtocolArgs(std::string protocol) { protocol_ = std::move(protocol); return *this; } From 9ebae7615f02dab4c90eea28a8ca5efd52b67f8e Mon Sep 17 00:00:00 2001 From: yuyang Date: Fri, 7 Nov 2025 11:49:25 +0800 Subject: [PATCH 10/13] reformat the code --- mooncake-store/src/client.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mooncake-store/src/client.cpp b/mooncake-store/src/client.cpp index 821d5e19b..4587f59c7 100644 --- a/mooncake-store/src/client.cpp +++ b/mooncake-store/src/client.cpp @@ -419,7 +419,8 @@ MooncakeStoreBuilder& MooncakeStoreBuilder::WithMetadataConnectionString( return *this; } -MooncakeStoreBuilder& MooncakeStoreBuilder::WithProtocolArgs(std::string protocol) { +MooncakeStoreBuilder& MooncakeStoreBuilder::WithProtocolArgs( + std::string protocol) { protocol_ = std::move(protocol); return *this; } From 1daeb48cfc3b04e8bada0158aeb73f3cfdd05923 Mon Sep 17 00:00:00 2001 From: yuyang Date: Fri, 7 Nov 2025 15:58:13 +0800 Subject: [PATCH 11/13] fixed names --- mooncake-store/src/client.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mooncake-store/src/client.cpp b/mooncake-store/src/client.cpp index 4587f59c7..122b2eb66 100644 --- a/mooncake-store/src/client.cpp +++ b/mooncake-store/src/client.cpp @@ -428,7 +428,7 @@ MooncakeStoreBuilder& MooncakeStoreBuilder::WithProtocolArgs( MooncakeStoreBuilder& MooncakeStoreBuilder::WithTransferEngineArgs( std::string engine_args) { // Can add some other engine arguments - device_names_ = std::move(engine_args); + engine_args_ = std::move(engine_args); return *this; } @@ -474,7 +474,7 @@ tl::expected, std::string> MooncakeStoreBuilder::Build() } auto client = Client::Create(*local_hostname_, *metadata_connstring_, protocol_, - device_names_, master_server_entry_, transfer_engine_); + engine_args_, master_server_entry_, transfer_engine_); if (!client) { std::string error_msg = "Client creation failed"; return tl::make_unexpected(std::move(error_msg)); From 1780a31ad04b32f190b58815a10f1e37fcde190c Mon Sep 17 00:00:00 2001 From: yuyang Date: Mon, 10 Nov 2025 10:42:29 +0800 Subject: [PATCH 12/13] fixed names of Protocol and ProtocolArgs --- mooncake-store/include/client.h | 6 +++--- mooncake-store/src/client.cpp | 11 +++++------ 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/mooncake-store/include/client.h b/mooncake-store/include/client.h index 4578c688e..00239d63b 100644 --- a/mooncake-store/include/client.h +++ b/mooncake-store/include/client.h @@ -376,8 +376,8 @@ class MooncakeStoreBuilder { MooncakeStoreBuilder& WithLocalHostname(std::string local_hostname); MooncakeStoreBuilder& WithMetadataConnectionString( std::string metadata_connstring); - MooncakeStoreBuilder& WithProtocolArgs(std::string protocol); - MooncakeStoreBuilder& WithTransferEngineArgs(std::string engine_args); + MooncakeStoreBuilder& WithProtocol(std::string protocol); + MooncakeStoreBuilder& WithProtocolArgs(std::string protocol_args); MooncakeStoreBuilder& WithMasterEndpoint(std::string master_server_entry); MooncakeStoreBuilder& WithExistingTransferEngine( std::shared_ptr transfer_engine); @@ -389,7 +389,7 @@ class MooncakeStoreBuilder { std::optional local_hostname_; std::optional metadata_connstring_; std::string protocol_ = "tcp"; - std::optional engine_args_; + std::optional protocol_args_; std::string master_server_entry_ = kDefaultMasterAddress; std::shared_ptr transfer_engine_ = nullptr; }; diff --git a/mooncake-store/src/client.cpp b/mooncake-store/src/client.cpp index 122b2eb66..9a02c7ac7 100644 --- a/mooncake-store/src/client.cpp +++ b/mooncake-store/src/client.cpp @@ -419,16 +419,15 @@ MooncakeStoreBuilder& MooncakeStoreBuilder::WithMetadataConnectionString( return *this; } -MooncakeStoreBuilder& MooncakeStoreBuilder::WithProtocolArgs( - std::string protocol) { +MooncakeStoreBuilder& MooncakeStoreBuilder::WithProtocol(std::string protocol) { protocol_ = std::move(protocol); return *this; } -MooncakeStoreBuilder& MooncakeStoreBuilder::WithTransferEngineArgs( - std::string engine_args) { +MooncakeStoreBuilder& MooncakeStoreBuilder::WithProtocolArgs( + std::string protocol_args) { // Can add some other engine arguments - engine_args_ = std::move(engine_args); + protocol_args_ = std::move(protocol_args); return *this; } @@ -474,7 +473,7 @@ tl::expected, std::string> MooncakeStoreBuilder::Build() } auto client = Client::Create(*local_hostname_, *metadata_connstring_, protocol_, - engine_args_, master_server_entry_, transfer_engine_); + protocol_args_, master_server_entry_, transfer_engine_); if (!client) { std::string error_msg = "Client creation failed"; return tl::make_unexpected(std::move(error_msg)); From 67d12680b3fd2cf545f94403665ca73fef7fb529 Mon Sep 17 00:00:00 2001 From: yuyang Date: Mon, 10 Nov 2025 10:51:55 +0800 Subject: [PATCH 13/13] Reformat the codes --- mooncake-store/include/pybind_client.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/mooncake-store/include/pybind_client.h b/mooncake-store/include/pybind_client.h index 31f0e5dd5..d79c0aab0 100644 --- a/mooncake-store/include/pybind_client.h +++ b/mooncake-store/include/pybind_client.h @@ -21,9 +21,8 @@ constexpr bool is_supported_return_type_v = std::is_void_v || std::is_integral_v; template -requires is_supported_return_type_v int64_t -to_py_ret(const tl::expected &exp) -noexcept { + requires is_supported_return_type_v +int64_t to_py_ret(const tl::expected &exp) noexcept { if (!exp) { return static_cast(toInt(exp.error())); }