-
Notifications
You must be signed in to change notification settings - Fork 804
[docs][v0.11.0-dev]Add KV Pool feature guide #5155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: SlightwindSec <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds a new feature guide for KV Pool with Mooncake, replacing an older deployment guide. The new documentation is comprehensive, covering environment setup, configuration, and various deployment scenarios. My review focuses on ensuring the correctness of the provided commands to prevent user errors. I've identified and suggested fixes for a couple of critical errors in shell commands within the guide that would otherwise fail when executed.
|
|
||
| An example command for compiling ADXL: | ||
|
|
||
| `rm -rf build && mkdir -p build && cd build \ && cmake .. -DCMAKE_INSTALL_PREFIX=/opt/transfer-engine/ -DCMAKE_POLICY_VERSION_MINIMUM=3.5 -DUSE_ASCEND_DIRECT=ON -DBUILD_SHARED_LIBS=ON -DBUILD_UNIT_TESTS=OFF \ && make -j \ && make install` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compilation command contains misplaced backslashes (\) that will cause it to fail if copied and pasted. The backslashes appear to be intended for line continuation but are incorrectly placed before &&. This will cause the shell to interpret the command incorrectly. To fix this, the backslashes should be removed to form a single, valid command line.
| `rm -rf build && mkdir -p build && cd build \ && cmake .. -DCMAKE_INSTALL_PREFIX=/opt/transfer-engine/ -DCMAKE_POLICY_VERSION_MINIMUM=3.5 -DUSE_ASCEND_DIRECT=ON -DBUILD_SHARED_LIBS=ON -DBUILD_UNIT_TESTS=OFF \ && make -j \ && make install` | |
| `rm -rf build && mkdir -p build && cd build && cmake .. -DCMAKE_INSTALL_PREFIX=/opt/transfer-engine/ -DCMAKE_POLICY_VERSION_MINIMUM=3.5 -DUSE_ASCEND_DIRECT=ON -DBUILD_SHARED_LIBS=ON -DBUILD_UNIT_TESTS=OFF && make -j && make install` |
| --host localhost\ | ||
| --prefiller-hosts localhost \ | ||
| --prefiller-ports 8100 \ | ||
| --decoder-hosts localhost\ | ||
| --decoder-ports 8200 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command to start the proxy server has incorrect line continuations.
- On lines 214 and 217, the backslash
\is not separated from the argument by a space. This makes the backslash part of the argument value (e.g.,localhost\), which will cause errors. - On line 218, there is a trailing backslash which is unnecessary for the last argument line.
I've corrected these issues in the suggestion.
| --host localhost\ | |
| --prefiller-hosts localhost \ | |
| --prefiller-ports 8100 \ | |
| --decoder-hosts localhost\ | |
| --decoder-ports 8200 \ | |
| --host localhost \ | |
| --prefiller-hosts localhost \ | |
| --prefiller-ports 8100 \ | |
| --decoder-hosts localhost \ | |
| --decoder-ports 8200 |
Signed-off-by: SlightwindSec <[email protected]>
Signed-off-by: SlightwindSec <[email protected]>
Signed-off-by: SlightwindSec <[email protected]>
Signed-off-by: SlightwindSec <[email protected]>
Signed-off-by: SlightwindSec <[email protected]>
|
@LCAIZJ Please merge this if it's fine |
Sure, I'll take care of reviewing and merging this PR. |
|
Could you please take a moment to reply to the question above? @SlightwindSec |
Signed-off-by: SlightwindSec <[email protected]>
Done. |
|
Please merge this if it's fine @LCAIZJ , thanks! |
Add KV Pool feature guide