[TransferEngine] Support Status return value #125
Conversation
|
This PR is to replace #118. |
yuan-luo
force-pushed
the
support_status_prod
branch
from
d2d4f9d to
4beb1ee
Compare
There was a problem hiding this comment.
If this the continue part of #118 , we'd better use push new commits in 118, instead of open a new PR. Or, close 118 could be better too.
| @@ -57,6 +57,31 @@ int TransferEngine::freeEngine() { | |||
| return 0; | |||
| } | |||
|
|
|||
| int TransferEngine::submitTransfer(BatchID batch_id, | |||
| const std::vector<TransferRequest> &entries) { | |||
| // TODO: return Status in public function. | |||
There was a problem hiding this comment.
could you explain the plan to reach it? seems we just use Status in internal APIs?
There was a problem hiding this comment.
The plan is to replace the return value in the public functions, so as the return values will not conflict with each other between cross components. The reason to set TODO here is because the complete converting work is non-trivial, which may impact the vLLM adapter in the Mooncake upstream. So I send out this PR to collect some review comments.
Were this approach approved, I'd like to change all the external APIs' return value into Status.
There was a problem hiding this comment.
okay, that make sense, the exposed C APIs need to take care too.
There was a problem hiding this comment.
okay, I'll proceed with this PR.
There was a problem hiding this comment.
@doujiang24 Please take another look at this PR. Thanks.
yuan-luo
force-pushed
the
support_status_prod
branch
2 times, most recently
from
8e29625 to
5df40a7
Compare
(cherry picked from commit 2cae769)
yuan-luo
force-pushed
the
support_status_prod
branch
from
5df40a7 to
b9733af
Compare
This PR implements Status return value in the TransferEngine.
This is for better debugging and is able to avoid cross-module error code conflict.