github-actions[bot] commented on code in PR #69554:
URL: https://github.com/apache/doris/pull/69554#discussion_r4370332388
##########
be/src/exec/sink/writer/vtablet_writer.cpp:
##########
@@ -224,21 +242,28 @@ void IndexChannel::mark_as_failed(const VNodeChannel*
node_channel, const std::s
}
int IndexChannel::_max_failed_replicas(int64_t tablet_id) {
- auto [total_replicas_num, load_required_replicas_num] =
- _parent->_tablet_replica_info[tablet_id];
- int max_failed_replicas = total_replicas_num == 0
- ? (_parent->_num_replicas - 1) / 2
- : total_replicas_num -
load_required_replicas_num;
- return max_failed_replicas;
+ DORIS_CHECK(_parent->_tablet_replica_info.contains(tablet_id));
Review Comment:
[P1] Handle tablet errors from partitions opened by another sender
A multi-instance automatic-partition load can legitimately reach this
assertion: W1 dynamically opens partition A, W2 opens B on the same backend,
and W2 sends the final EOS. `TabletsChannel::close()` unions the senders'
touched partitions, so a rowset-build/delete-bitmap/commit error for A is
returned to W2 by `_close_writers()`. W2's `_add_block_success_callback()`
calls `mark_as_failed(A)`; the backend exists in its channel map for B, but A
is absent from its private `_tablet_replica_info` because partition callbacks
update only the requesting writer. This new check therefore aborts debug/ASAN
BEs; in Release it throws out of the RPC callback before `clear_in_flight()`,
with no Doris catch boundary. Previously the missing entry used the table-level
fallback. Make the replica policy available for the full receiver-result scope,
or handle nonlocal tablet errors without asserting local ownership, and add a
two-sender automatic-partition late-error regression. This is distinct from
the earlier selected-replica row-loss findings.
##########
be/src/exec/sink/writer/vtablet_writer_v2.cpp:
##########
@@ -1103,10 +1147,22 @@ Status
VTabletWriterV2::_create_commit_info(std::vector<TTabletCommitInfo>& tabl
gap_it != _tablet_version_gap_backends.end()) {
failed_backends.insert(gap_it->second.begin(),
gap_it->second.end());
}
- auto [total_replicas_num, load_required_replicas_num] =
_tablet_replica_info[tablet_id];
- int max_failed_replicas = total_replicas_num == 0
- ? (_num_replicas - 1) / 2
- : total_replicas_num -
load_required_replicas_num;
+ auto replica_info_it = _tablet_replica_info.find(tablet_id);
+ if (replica_info_it == _tablet_replica_info.end()) {
Review Comment:
[P2] Share incremental replica metadata with pooled close results
With V2, two local sink instances share a `LoadStreamMap`, but
`on_partitions_created()` populates replica information only in the instance
that requested the new partition. Let W1 alone discover automatic partition A
with three selected replicas and required=2, while W2 processes another
partition. If A has one late tablet-close failure and two successful replicas,
W2 can be the last local closer and run `_create_commit_info()` over the pooled
results. A is then missing from W2's private map, so this new branch rejects a
load whose quorum succeeded; the previous fallback tolerated that single
failure. Store/merge incremental replica requirements with the shared result
ownership instead of requiring the arbitrary final writer to know every tablet,
and cover distinct automatic partitions across multiple local sinks. This is
the separate V2 implementation, distinct from the existing selected-target
row-loss thread and the legacy fatal assertion.
##########
be/src/exec/sink/writer/vtablet_writer_v2.cpp:
##########
@@ -357,31 +358,73 @@ Status VTabletWriterV2::_build_tablet_node_mapping() {
_indexes_from_node[node].emplace_back(tablet);
known_indexes.insert(index.index_id);
}
- _build_tablet_replica_info(tablet_id, partition);
+ RETURN_IF_ERROR(_build_tablet_replica_info(tablet_id,
partition));
}
}
}
return Status::OK();
}
-void VTabletWriterV2::_build_tablet_replica_info(const int64_t tablet_id,
- VOlapTablePartition*
partition) {
+Status VTabletWriterV2::_build_tablet_replica_info(const int64_t tablet_id,
+ VOlapTablePartition*
partition) {
+ const auto* tablet_location = _location->find_tablet(tablet_id);
+ if (tablet_location == nullptr) {
+ return Status::InternalError("unknown tablet location, tablet_id={}",
tablet_id);
+ }
+ int selected_replicas_num =
cast_set<int>(tablet_location->node_ids.size());
+ constexpr int64_t DUMMY_TABLET_ID = 0;
+ if (tablet_id == DUMMY_TABLET_ID) [[unlikely]] {
+ if (selected_replicas_num != 1) {
+ return Status::InternalError(
+ "bootstrap tablet {} must have exactly one selected
replica, actual {}",
+ tablet_id, selected_replicas_num);
+ }
+ _tablet_replica_info[tablet_id] = TabletReplicaInfo
{.total_replicas_num = 1,
+
.load_required_replicas_num = 1,
+
.selected_replicas_num = 1};
+ return Status::OK();
+ }
if (partition != nullptr) {
int total_replicas_num =
partition->total_replica_num == 0 ? _num_replicas :
partition->total_replica_num;
int load_required_replicas_num = partition->load_required_replica_num
== 0
? (_num_replicas + 1) / 2
:
partition->load_required_replica_num;
+ if (selected_replicas_num < load_required_replicas_num) {
Review Comment:
[P2] Preserve minimum-replica loads with older FE partition responses
During an old-master/new-BE rolling upgrade, `createPartition` responses
omit the optional `total_replica_num` and `load_required_replica_num` fields
that this PR starts sending. They remain zero in `VOlapTablePartition`, so the
preceding fallback turns a table with replication=3 and
`min_load_replica_num=1` into required=2. If two replicas become unavailable
after a dynamic partition is created but before the older FE selects its
locations, it returns the remaining healthy destination (the old RPC only logs
the majority shortfall). This new check now rejects selected=1, although the
previous writer accepted it and FE commit validation requires only one success.
The legacy writer's new `IndexChannel::init()` check has the same regression.
Sending required=1 from the new FE fixes fully upgraded clusters, but does not
protect older masters. Preserve compatible handling of omitted requirements, or
recover the actual minimum before enforcing the check, and test an
old-FE-shaped dynami
c response.
--
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]
