Userwhite commented on PR #53144: URL: https://github.com/apache/doris/pull/53144#issuecomment-3732379280
> ## 代码审查：流加载异步返回优化 > 感谢您提交的 PR！优化思路很合理——异步处理 HTTP 回复应该可以提高高并发环境下的线程利用率。我已经跟踪了同步逻辑，并验证了它能够正确处理竞态条件。 > > ### 关键问题（请在合并前修复） > #### 1. 变量名拼写错误（`http_request.h:108`） > ```c++ > std::future<bool> _http_reply_futrue = _http_reply_promise.get_future(); > // ^^^^^^ should be "future" > ``` > > #### 2. 硬编码的 3 秒超时时间 ( `http_request.cpp:175`) > ```c++ > auto status = _http_reply_futrue.wait_for(std::chrono::seconds(3)); > ``` > > 3 秒对于真正需要长时间运行的负载来说太短了。请考虑以下几点： > > * 使其可配置`config::stream_load_async_reply_timeout_s` > * 默认值为 300 秒左右。 > > #### 3. 调试堆栈跟踪日志记录 ( `http_request.cpp:159`) > ```c++ > VLOG_NOTICE << "finish send reply, infos=" << infos > << ", stack=" << get_stack_trace(); // temp locate problem > ``` > > * `get_stack_trace()`即使禁用 VLOG，开销仍然很大（字符串仍然会被构建）。 > * 评论指出“临时定位问题”——合并前应删除。 > > ### 小问题 > 1. **重复错误日志记录**- 同一错误记录在`handle()`第 122 行和`_send_reply()`第 157 行。 > 2. **漏接`finish_send_reply()`了`send_error()`**——`http_channel.cpp`拨打电话`finish_send_reply()`时漏接了`send_reply()`，`send_file()`但拨打其他电话时漏接了`send_error()`。这是故意的吗？ > 3. **建议`is_prepare_success`**——虽然可以使用`shared_ptr<bool>`，但`std::atomic<bool>`可以更清晰地表达意图。 > 4. **建议添加注释**`_can_send_reply`——包含、`_finish_send_reply`和 CV 的同步状态机是正确的，但比较复杂。添加解释等待条件的注释将有助于未来的维护者。 > > ### 螺纹安全分析 ✅ > 我逐一分析了这些场景，并验证了逻辑是正确的： > > 设想 结果 > 正常成功路径 ✅ 通过回调发送回复 > `on_header`失败 ✅ 立即回复，`handle()`跳过 > 碎片准备失败 ✅ 回调函数未被调用，`handle()`发送回复 > 请求超时 ✅`_finish_send_reply=true`防止免费期满后使用 > ### 全面的 > **概念**：✅ 声音优化方法 **实现**：⚠️需要改进（拼写错误、超时、调试日志） **线程安全**：✅ 正确 > > 建议在合并前解决这3个关键问题。 std::atomic > * **建议`is_prepare_success`**——虽然可以使用`shared_ptr<bool>`，但`std::atomic<bool>`可以更清楚地表达意思。 > * is_prepare_success must use std::shared_ptr s_prepare_success is freed after _exec_env->fragment_mgr()->exec_plan_fragment() is called, but the exec_fragment callback still needs to access it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
