github-actions[bot] commented on code in PR #66680:
URL: https://github.com/apache/doris/pull/66680#discussion_r3766972026
##########
fe/fe-core/src/main/java/org/apache/doris/transaction/DatabaseTransactionMgr.java:
##########
@@ -494,6 +495,8 @@ private void checkCommitStatus(List<Table> tableList,
TransactionState transacti
TabletInvertedIndex tabletInvertedIndex = env.getTabletInvertedIndex();
Map<Long, Set<Long>> tabletToBackends = new HashMap<>();
Map<Long, Table> idToTable = new HashMap<>();
+ Map<String, Integer> crossAzSuccQuorum = Config.getCrossAzSuccQuorum();
Review Comment:
This rereads a mutable policy and live backend locations at commit, but BE
close waiting used the policy and locations serialized when the sink was
planned. A reachable failure is: plan with the default empty config, delay the
only az2 replica, then enable `az1:2,az2:1` before close/commit. BE has no AZ
gate and may stop after the two az1 replicas; this fresh read then requires az2
and rejects an otherwise healthy load that would have succeeded if BE kept
waiting. Disabling the config or changing a location tag in flight creates the
inverse mismatch, and the separate reads inside `setNodesInfo` can even pair an
enabled policy with location-less nodes. Please bind one immutable
policy/location snapshot to the transaction/load and use it for node
construction, sink metadata (including auto-partition updates), and FE
validation; add blocked in-flight enable/disable coverage.
##########
be/src/exec/sink/writer/vtablet_writer_v2.cpp:
##########
@@ -945,6 +952,22 @@ bool VTabletWriterV2::_quorum_success(
return false;
}
}
+ if (table_sink.__isset.cross_az_succ_quorum) {
Review Comment:
This enforces the full AZ requirement during
`_close_wait(_non_incremental_streams(), false)`, before incremental streams
are sent `CLOSE_LOAD`. For an auto-created tablet whose required-AZ success is
available only on an incremental destination, that success cannot exist in this
stage; if any extra initial stream is slow, the loop cannot use quorum and can
run to the query timeout before reaching `close_load(true)`. The v1 two-stage
path has the same dependency. Please make the first-stage fence depend only on
evidence that can finish in that stage and enforce the full AZ rule in the
final all-stream wait; add auto-partition tests for both writers.
##########
be/src/exec/sink/writer/vtablet_writer_v2.cpp:
##########
@@ -922,6 +924,11 @@ bool VTabletWriterV2::_quorum_success(
}
if (finished) {
finished_dst_ids.insert(dst_id);
+ if (table_sink.__isset.cross_az_succ_quorum) {
+ for (int64_t tablet_id : streams->success_tablets()) {
Review Comment:
`success_tablets()` is source-local, but the destination sends final tablet
IDs only on whichever source's `CLOSE_LOAD` arrives last. Earlier sources
receive an empty EOS and are only stream-closed later; the existing two-client
load-stream test even expects only the second client to get the tablet ID.
Consequently another source can have ordinary quorum and a genuinely satisfied
AZ requirement, yet keep waiting for an unrelated slow destination because this
map is empty, eventually timing out instead of using quorum close. Please
share/broadcast final tablet outcomes to every source that evaluates this
predicate (or evaluate it at the aggregated owner) and add a multi-source v2
close test.
##########
be/src/exec/sink/writer/vtablet_writer.cpp:
##########
@@ -492,6 +492,44 @@ bool IndexChannel::_quorum_success(const
std::unordered_set<int64_t>& unfinished
}
}
+ const auto& table_sink = _parent->_t_sink.olap_table_sink;
+ if (table_sink.__isset.cross_az_succ_quorum) {
+ std::unordered_set<int64_t> finished_node_ids;
+ for (const auto& [node_id, node_channel] : _node_channels) {
+ if (!unfinished_node_channel_ids.contains(node_id) &&
+ node_channel->check_status().ok()) {
+ finished_node_ids.insert(node_id);
+ }
+ }
+ for (int64_t tablet_id : need_finish_tablets) {
+ const auto* tablet = _parent->_location->find_tablet(tablet_id);
+ if (tablet == nullptr) {
+ continue;
+ }
+ std::unordered_set<int64_t> successful_node_ids;
+ {
+ std::lock_guard<std::mutex> l(_fail_lock);
+ const auto failed_it = _failed_channels.find(tablet_id);
+ for (int64_t node_id : tablet->node_ids) {
+ if (finished_node_ids.contains(node_id) &&
+ (failed_it == _failed_channels.end() ||
Review Comment:
This still treats absence from this sender's `_failed_channels` as tablet
success, but non-final senders never receive the final tablet outcome.
`TabletsChannel::close` returns them OK with empty
`tablet_errors`/`tablet_vec`; only the sender that closes the receiver last
gets commit errors, and `hang_wait` does not copy that response back. Thus an
earlier sender can count a destination whose tablet later fails in a required
AZ, enter the bounded post-quorum wait, and cancel the only slow healthy
replica before FE rejects. Please publish the final per-tablet result to every
sender that evaluates quorum (or centralize the decision) and add a two-sender
v1 case where the non-final sender closes first.
##########
fe/fe-core/src/main/java/org/apache/doris/transaction/DatabaseTransactionMgr.java:
##########
@@ -670,6 +679,41 @@ private void checkCommitStatus(List<Table> tableList,
TransactionState transacti
throw new
TabletQuorumFailedException(transactionId, errMsg);
}
+
+ for (Entry<String, Integer> entry :
crossAzSuccQuorum.entrySet()) {
+ String az = entry.getKey();
+ int replicaNumInAz = 0;
+ for (long backendId : tabletBackends) {
Review Comment:
`tabletBackends` includes every replica object, including a scheduler-added
`CLONE`, but load planning excludes `CLONE` because it cannot load. With one
allocated az1 replica and `az1:2`, a same-AZ repair/balance temporarily leaves
the loadable replica plus the new CLONE here, so `requiredInAz` becomes 2
although only one replica was a write target; every otherwise valid load is
rejected until cloning finishes. Please clamp against the declared/logical
replica allocation (or another count that excludes transient extra replicas)
and add a one-replica-plus-CLONE test.
--
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]
