github-actions[bot] commented on code in PR #67759:
URL: https://github.com/apache/doris/pull/67759#discussion_r3981619811
##########
be/src/load/delta_writer/delta_writer.cpp:
##########
@@ -136,7 +142,14 @@ int64_t BaseDeltaWriter::table_id() const {
DeltaWriter::~DeltaWriter() = default;
+Status BaseDeltaWriter::_get_load_cancel_status() const {
+ return _req.load_cancel_status && !_req.load_cancel_status->ok()
+ ? _req.load_cancel_status->status()
+ : Status::OK();
+}
+
Status BaseDeltaWriter::init() {
+ RETURN_IF_ERROR(_get_load_cancel_status());
Review Comment:
[P2] Recheck cancellation inside cloud rowset initialization
This gate only covers entry to init(). With cloud MoW sync enabled by
default, CloudRowsetBuilder::init() can block in sync_rowsets(); if cancel
publishes and returns there, the same retained task then creates the writer and
starts a separate prepare_rowset RPC without rechecking.
CloudGroupRowsetBuilder can similarly finish the row-binlog child's prepare
after cancellation and start the data child's sync/prepare. The existing
post-init writer thread only gates the later memtable call, and the
empty-rowset thread only gates prepare-to-commit, so neither prevents this
post-cancel prepare. Existing failure exits already tolerate these partial-init
states. Consume the copied load status at safe boundaries after sync and before
prepare, and between group child inits, with barriers asserting no later
prepare runs after completed cancel.
##########
be/src/load/channel/load_channel.cpp:
##########
@@ -180,6 +183,7 @@ Status
LoadChannel::_get_tablets_channel(std::shared_ptr<BaseTabletsChannel>& ch
Status LoadChannel::add_batch(const PTabletWriterAddBlockRequest& request,
PTabletWriterAddBlockResult* response) {
+ RETURN_IF_ERROR(cancel_status());
Review Comment:
[P2] Recheck cancellation before acknowledging retained RPC work
This is only an entry gate. An open can pass it and the child gate, then a
concurrent cancel can remove the manager entry, publish the status, and return
while child setup is still running; open subsequently sets _opened and returns
OK. Non-EOS add has the same response-boundary gap, and it is stronger than an
unavoidable final-sample race: if DeltaWriter::write() returns the shared
cancellation, _write_block_data() converts it to tablet_errors, advances
_next_seqs, and returns OK, so the RPC acknowledges a cancellation it actually
observed. The existing DeltaWriter thread covers pre-operation gates after
init/backpressure, not this parent conversion/return boundary. Please make the
successful open/add transition coherent with the shared terminal state,
preserve an already-observed load-wide cancellation as the RPC result, and add
barriers proving a completed cancel cannot be followed by an OK open/add
response.
##########
be/src/load/channel/load_channel_mgr.cpp:
##########
@@ -203,41 +233,47 @@ void LoadChannelMgr::_finish_load_channel(const UniqueId
load_id) {
Status LoadChannelMgr::cancel(const PTabletWriterCancelRequest& params) {
UniqueId load_id(params.id());
+ const auto reason = params.has_cancel_reason() &&
!params.cancel_reason().empty()
+ ? params.cancel_reason()
+ : "load channel cancelled";
std::shared_ptr<LoadChannel> cancelled_channel;
{
std::lock_guard<std::mutex> l(_lock);
if (_load_channels.contains(load_id)) {
cancelled_channel = _load_channels[load_id];
_load_channels.erase(load_id);
}
- // We just need to record the first cancel msg
- auto* existing_handle =
_load_state_channels->lookup(load_id.to_string());
- if (existing_handle == nullptr) {
- if (params.has_cancel_reason() && !params.cancel_reason().empty())
{
- std::unique_ptr<CacheValue> cancel_reason_ptr =
std::make_unique<CacheValue>();
- cancel_reason_ptr->_cancel_reason = params.cancel_reason();
- size_t cache_capacity =
- cancel_reason_ptr->_cancel_reason.capacity() +
sizeof(CacheValue);
- auto* handle = _load_state_channels->insert(
- load_id.to_string(), cancel_reason_ptr.get(), 1,
cache_capacity);
- cancel_reason_ptr.release();
- _load_state_channels->release(handle);
- LOG(INFO) << fmt::format("load_id = {}, record_error reason =
{}",
- print_id(load_id),
params.cancel_reason());
- }
- } else {
- _load_state_channels->release(existing_handle);
- }
+ _record_cancelled_load_channel(load_id, reason);
Review Comment:
[P2] Cache the retained channel's actual first failure
_cancel_load_channel() can publish failure A and pause before _lock. Public
cancel then wins _lock, erases the same channel, and records reason B here
before calling cancelled_channel->cancel(B). That later update loses to
first-wins A; when the failure path resumes it sees the mapping gone and cannot
repair the cache. Retained operations report A while late open/add reports B,
so the manager tombstone no longer reflects the first terminal state. This is
distinct from the existing EOS/replacement thread: exact-instance publication
has already happened, but public cancel chooses the cache value before
consulting it. Publish to the captured channel before selecting the cache value
and store its effective first status while keeping the exact-instance
transition, with a barrier test for the A-published/B-cancel ordering.
--
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]
