pass transfer engine to mooncake store to reuse the engine to transfer data #899
Conversation
hjchen2
changed the title
|
Good thought. I have a similar idea but lack the time to implement it. Will review it ASAP. |
|
/gemini review |
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the MooncakeStore and its underlying Client to allow reusing an existing TransferEngine instance. This is achieved by changing the Client to hold a std::shared_ptr<TransferEngine> instead of a direct instance, and adding new API overloads in both C++ and Python bindings to accept a pre-initialized engine. The changes are well-implemented and follow the intended goal. I've found one instance of redundant code that can be removed to improve maintainability.
mooncake-store/src/pybind_client.cpp
Outdated
| if (transfer_engine && | ||
| local_hostname != transfer_engine.value()->getLocalIpAndPort()) { | ||
| LOG(ERROR) << "the local_hostname should be consistent with the " | ||
| "transfer engine local ip and port. local_hostname is " | ||
| << local_hostname | ||
| << ", but the transfer engine local ip and port is " | ||
| << transfer_engine.value()->getLocalIpAndPort(); | ||
| return tl::unexpected(ErrorCode::INVALID_PARAMS); | ||
| } |
There was a problem hiding this comment.
This check for local_hostname consistency is redundant as it's already performed in Client::InitTransferEngine. In the new setup overload path, this condition will always be false, making it dead code for that path. To avoid code duplication and improve maintainability, this check can be removed from here. The check in Client::InitTransferEngine is sufficient.
3 participants
No description provided.