refactor: introduce expected pattern for error handling in master service by xiaguan · Pull Request #562 · kvcache-ai/Mooncake

xiaguan · 2025-06-26T09:11:44Z

Overview

This PR introduces the tl::expected pattern for error handling in the master service, replacing traditional ErrorCode return types with a more type-safe and explicit approach.

Changes

Core Improvements

Type-safe error handling: Replace ErrorCode return types with tl::expected<T, ErrorCode> pattern
Clearer API semantics: Success values and error codes are now explicitly separated
Better developer experience:
- Success cases: access via .value()
- Error cases: access via .error()
- Eliminates ambiguity between success and error states

Modified Components

MasterService methods: Updated to return expected<void, ErrorCode> or expected<T, ErrorCode>
RPC service interfaces: Modified to support the expected pattern
Test suites: Updated all related tests to handle new expected return types
Dependencies: Added necessary includes for ylt/util/expected.hpp

Files Changed

mooncake-store/include/master_service.h - Updated method signatures
mooncake-store/include/rpc_service.h - RPC interface adaptations
mooncake-store/include/types.h - Minor formatting improvements
mooncake-store/src/master_service.cpp - Implementation updates
mooncake-store/tests/ - Comprehensive test updates

Benefits

Type Safety: Compile-time guarantees about error handling
Explicit Error Handling: No more ambiguous return codes
Better Maintainability: Clear separation of success and error paths
Future-Ready: Foundation for extending expected pattern to RPC responses

Future Work

This PR lays the groundwork for:

Extending expected pattern to RPC response types
Enhancing the error code system for more comprehensive error handling
Further improving type safety across the codebase

Testing

All existing tests have been updated and continue to pass, ensuring backward compatibility and correctness of the new error handling approach.

Note: This change maintains API compatibility while improving error handling semantics. The expected pattern provides a modern, type-safe approach to error handling that will benefit future development and maintenance.

Copilot

Pull Request Overview

This PR modernizes error handling across the master service and client layers by adopting tl::expected<T, ErrorCode>, updating method signatures, RPC adapters, and test suites to use has_value()/.error().

Core RPC and service methods now return tl::expected instead of ErrorCode.
Test code is updated to assert on .has_value() and extract .error() where appropriate.
Introduced execute_rpc helper in rpc_helper.h and enhanced logging in scoped_vlog_timer.h.

Reviewed Changes

Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/stress_workload_test.cpp	Updated mount/unmount calls to use `.has_value()` checks
tests/client_integration_test.cpp	Refactored client calls to expected pattern; minor typo introduced
tests/master_service_test.cpp	Converted `EXPECT_EQ` on `ErrorCode` to `EXPECT_TRUE`/`.error()` assertions
include/rpc_helper.h	Added generic `execute_rpc` helper for RPC logging and metrics
include/utils/scoped_vlog_timer.h	Enhanced `ScopedVLogTimer` with `LogResponseExpected` for `tl::expected`

ykwd

Great work! Just a few suggestions in the comments.

ykwd · 2025-06-30T08:30:42Z

+        // Special handling for bool to log the value, similar to the original
+        // implementation of ExistKey.
+        if constexpr (std::is_same_v<ResultType, bool>) {
+            timer.LogResponse("success=true, exist=", result.value());


Using success=true or false is a little bit ambiguous. Also, the "exist=" may only be suitable if the returned value denotes whether something exists or not. How about timer.LogResponse("status=success, return=", result.value());

Good suggestion. Resolved.

ykwd · 2025-06-30T08:31:05Z

+        if constexpr (std::is_same_v<ResultType, bool>) {
+            timer.LogResponse("success=true, exist=", result.value());
+        } else {
+            timer.LogResponse("success=true");


timer.LogResponse("status=success“);

Good suggestion. Resolved.

ykwd · 2025-06-30T08:31:32Z

+    } else {
+        inc_fail_metric();
+        timer.LogResponse("success=false, error=", toString(result.error()));
+    }


timer.LogResponse("status=fail, error=", toString(result.error()));

ykwd · 2025-06-30T08:50:46Z

+auto execute_rpc(std::string_view rpc_name, RpcCallable&& rpc_call,
+                 LogRequestCallable&& log_request,
+                 std::function<void()> inc_req_metric,
+                 std::function<void()> inc_fail_metric) {


Just like the rpc_call and log_request, why don't we also use template for in_req_metric and inc_fail_metric? The template can prevent the overhead of the std::function

ykwd · 2025-06-30T08:53:40Z

-    }
+        std::vector<tl::expected<std::vector<Replica::Descriptor>, ErrorCode>>
+            results;
+        results.reserve(keys.size());


Will we record batchGet and batchPut metrics?

…vice - Replace ErrorCode return types with tl::expected<T, ErrorCode> pattern - Improve error handling clarity by separating success values from error codes - Update MasterService methods to return expected<void, ErrorCode> or expected<T, ErrorCode> - Modify RPC service interfaces to support expected pattern - Update all related tests to handle new expected return types - Add necessary includes for ylt/util/expected.hpp This change makes error handling more explicit and type-safe: - Success cases can be accessed via .value() - Error cases can be accessed via .error() - Eliminates ambiguity between success and error states Future work: - Extend expected pattern to RPC response types - Enhance error code system for more comprehensive error handling

…Code - Updated MasterService methods to replace ylt::expected with tl::expected for better error handling. - Modified BatchGetReplicaList, BatchPutStart, BatchPutEnd, and other methods to return tl::expected types. - Enhanced ClientIntegrationTest to handle expected results from Put, Get, Remove, and other operations using tl::expected. - Adjusted error handling in clientctl and master_metrics_test to utilize the new expected type. - Improved overall error reporting in tests to provide clearer feedback on operation failures.

- Enhanced client.h and client.cpp with new functionality - Removed obsolete master.proto file - Updated master_client.cpp and transfer_task.cpp - Improved integration and stress tests

- Replace BatchObjectInfo with vector<vector<Replica::Descriptor>> - Update BatchPut to handle new return type vector<tl::expected<void, ErrorCode>> - Fix BatchQuery API usage to work with new expected pattern - Convert unordered_map to vector format for BatchPut parameter compatibility

Resolved merge conflicts in multiple files: - mooncake-integration/store/store_py.cpp: Updated replica handling to support both memory and disk replicas - mooncake-store/include/client.h: Unified API signatures for Get and BatchPut methods - mooncake-store/include/rpc_service.h: Added response structures and fixed Ping method - mooncake-store/src/client.cpp: Adopted main branch implementation with storage backend support - mooncake-store/src/master_client.cpp: Used main branch version - mooncake-store/src/transfer_task.cpp: Used main branch version - mooncake-store/tests/*: Used main branch test implementations Key changes integrated: - Enhanced storage backend functionality - Ascend transport support - Improved error handling and batch operations - Updated test infrastructure

…vice (kvcache-ai#562) * refactor: introduce expected pattern for error handling in master service - Replace ErrorCode return types with tl::expected<T, ErrorCode> pattern - Improve error handling clarity by separating success values from error codes - Update MasterService methods to return expected<void, ErrorCode> or expected<T, ErrorCode> - Modify RPC service interfaces to support expected pattern - Update all related tests to handle new expected return types - Add necessary includes for ylt/util/expected.hpp This change makes error handling more explicit and type-safe: - Success cases can be accessed via .value() - Error cases can be accessed via .error() - Eliminates ambiguity between success and error states Future work: - Extend expected pattern to RPC response types - Enhance error code system for more comprehensive error handling * Refactor error handling to use expected<T,E> pattern instead of ErrorCode - Updated MasterService methods to replace ylt::expected with tl::expected for better error handling. - Modified BatchGetReplicaList, BatchPutStart, BatchPutEnd, and other methods to return tl::expected types. - Enhanced ClientIntegrationTest to handle expected results from Put, Get, Remove, and other operations using tl::expected. - Adjusted error handling in clientctl and master_metrics_test to utilize the new expected type. - Improved overall error reporting in tests to provide clearer feedback on operation failures. * fix test compile * refactor: update client implementation and remove master.proto - Enhanced client.h and client.cpp with new functionality - Removed obsolete master.proto file - Updated master_client.cpp and transfer_task.cpp - Improved integration and stress tests * fix: update Python integration to work with new batch API - Replace BatchObjectInfo with vector<vector<Replica::Descriptor>> - Update BatchPut to handle new return type vector<tl::expected<void, ErrorCode>> - Fix BatchQuery API usage to work with new expected pattern - Convert unordered_map to vector format for BatchPut parameter compatibility * refine master log and metric * fix(ci): should alloc first * merge main * fix tests

xiaguan force-pushed the master_expected branch 4 times, most recently from fa9d130 to a4a9a49 Compare June 30, 2025 06:17

xiaguan requested review from Copilot and ykwd and removed request for ykwd June 30, 2025 06:17

Copilot AI reviewed Jun 30, 2025

View reviewed changes

Comment thread mooncake-store/tests/stress_workload_test.cpp Outdated

Comment thread mooncake-store/tests/stress_workload_test.cpp Outdated

Comment thread mooncake-store/tests/client_integration_test.cpp Outdated

xiaguan force-pushed the master_expected branch from a4a9a49 to 5c8c4b3 Compare June 30, 2025 06:22

xiaguan self-assigned this Jun 30, 2025

xiaguan mentioned this pull request Jun 30, 2025

feat(store): add zero copy batch put and get for python binding #551

Merged

ykwd reviewed Jun 30, 2025

View reviewed changes

xiaguan force-pushed the master_expected branch from 91f2242 to deb325c Compare June 30, 2025 11:50

xiaguan added 6 commits June 30, 2025 19:52

fix test compile

0302716

refactor: update client implementation and remove master.proto

a61e14d

- Enhanced client.h and client.cpp with new functionality - Removed obsolete master.proto file - Updated master_client.cpp and transfer_task.cpp - Improved integration and stress tests

refine master log and metric

a9f1a5d

xiaguan force-pushed the master_expected branch from deb325c to a9f1a5d Compare June 30, 2025 12:16

fix(ci): should alloc first

3d0196f

xiaguan mentioned this pull request Jul 1, 2025

[bug] BatchIsExist and BatchIsPut may have errors. #571

Closed

ykwd approved these changes Jul 1, 2025

View reviewed changes

xiaguan added 3 commits July 3, 2025 10:57

merge main

2ae4bb6

fix tests

de2d387

xiaguan merged commit bde2fca into kvcache-ai:main Jul 3, 2025
10 checks passed

Conversation

xiaguan commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Changes

Core Improvements

Modified Components

Files Changed

Benefits

Future Work

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ykwd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xiaguan commented Jun 26, 2025 •

edited

Loading