[fix] add missing request type in blend server#2894
Conversation
Signed-off-by: deng451e <838677410@qq.com>
There was a problem hiding this comment.
Code Review
This pull request adds a PING request handler and includes it in the thread pool configuration within the run_cache_server function. Feedback suggests refactoring the handler registration logic into a shared function to address code duplication between different server implementations, which would improve the project's long-term maintainability and architectural soundness.
| server, RequestType.CB_RETRIEVE_PRE_COMPUTED_V2, engine.cb_retrieve_pre_computed | ||
| ) | ||
| add_handler_helper(server, RequestType.CB_STORE_FINAL, engine.cb_store_final) | ||
| add_handler_helper(server, RequestType.PING, engine.ping) |
There was a problem hiding this comment.
While adding the PING handler is correct, its manual addition points to a larger maintainability issue. The run_cache_server function is substantially duplicated between this file and lmcache/v1/multiprocess/server.py, leading to inconsistencies like this missing handler.
To improve the architecture and prevent future bugs, I recommend refactoring the common handler registration logic into a shared function. This function could be called by both run_cache_server implementations to ensure they stay synchronized.
References
- The style guide directs reviewers to focus on architectural soundness, modularity, and maintainability. The code duplication between
run_cache_serverimplementations represents a maintainability risk and a potential source of future bugs, which is a 'poor architectural decision' according to the guide's severity calibration. (link)
add missing request type in blend server Signed-off-by: deng451e <838677410@qq.com>
add missing request type in blend server Signed-off-by: deng451e <838677410@qq.com>
What this PR does / why we need it:
Special notes for your reviewers:
If applicable: