github-actions[bot] commented on code in PR #66680:
URL: https://github.com/apache/doris/pull/66680#discussion_r3775728654
##########
be/src/load/channel/load_channel_mgr.cpp:
##########
@@ -133,6 +137,32 @@ Status
LoadChannelMgr::_get_load_channel(std::shared_ptr<LoadChannel>& channel,
// load is success, success only when eos be true
_load_state_channels->release(handle);
if (request.has_eos() && request.eos()) {
+ if (request.need_final_tablet_result()) {
+ auto* result_handle =
+
_final_tablet_result_cache->lookup(load_id.to_string());
+ if (result_handle == nullptr) {
+ LOG(WARNING) << "Final tablet result is
unavailable for retried load "
+ << load_id << ", sender_id=" <<
request.sender_id();
+ is_eof = true;
+ return Status::OK();
+ }
+ const auto* result_cache_value =
+
reinterpret_cast<FinalTabletResultCache::CacheValue*>(
+
_final_tablet_result_cache->value(result_handle));
+ const auto result =
result_cache_value->_results.find(request.index_id());
+ if (result == result_cache_value->_results.end()) {
+ _final_tablet_result_cache->release(result_handle);
+ LOG(WARNING) << "Final tablet result is
unavailable for retried load "
+ << load_id << ", index_id=" <<
request.index_id()
+ << ", sender_id=" <<
request.sender_id();
+ is_eof = true;
+ return Status::OK();
+ }
+ response->CopyFrom(result->second.result);
Review Comment:
[P2] Copy the cached protobuf after releasing the manager lock
`_lock` is still held across this `CopyFrom`, and a cached final result can
approach the new 64 MiB limit. That allocation/copy blocks every unrelated
load-channel open, add-batch lookup, cancel, and finish on this BE, so one
retried large load can introduce load-wide RPC stalls. Retain the cache handle
(which pins the value) while resolving the state under `_lock`, release the
manager lock, then copy and release the handle; the finish-side cache eviction
work should likewise stay out of this global critical section.
##########
be/src/load/channel/load_channel_mgr.cpp:
##########
@@ -133,6 +137,32 @@ Status
LoadChannelMgr::_get_load_channel(std::shared_ptr<LoadChannel>& channel,
// load is success, success only when eos be true
_load_state_channels->release(handle);
if (request.has_eos() && request.eos()) {
+ if (request.need_final_tablet_result()) {
+ auto* result_handle =
+
_final_tablet_result_cache->lookup(load_id.to_string());
+ if (result_handle == nullptr) {
Review Comment:
[P1] Preserve the final result behind a success tombstone
This miss is reachable even without ordinary cache eviction:
`_finish_load_channel` publishes the null success tombstone before copying,
`cancel()` can erase the retained channel during that window while preserving
the tombstone, and the finisher then returns without inserting the result. The
bounded/prunable cache and oversized-result skip create the same state.
Returning `OK` here with an empty response makes the v1 sender finish without
successful-tablet ids or commit infos, so FE can reject a load whose replica
already committed. Please publish the tombstone and complete result atomically
with respect to cancel, and return a visible retry failure whenever the
complete result cannot be reproduced; the cancellation-window test should retry
and assert the full result.
--
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]
