github-actions[bot] commented on code in PR #66680:
URL: https://github.com/apache/doris/pull/66680#discussion_r3777494003
##########
be/src/exec/sink/writer/vtablet_writer_v2.cpp:
##########
@@ -1086,11 +1111,15 @@ Status
VTabletWriterV2::_create_commit_info(std::vector<TTabletCommitInfo>& tabl
failed_reason[tablet_id] = reason;
num_failed_tablets++;
}
+ const bool final_result_fanout =
_t_sink.olap_table_sink.__isset.cross_az_succ_quorum &&
+ streams.final_tablet_result_fanout();
for (auto tablet_id : streams.success_tablets()) {
- TTabletCommitInfo commit_info;
- commit_info.tabletId = tablet_id;
- commit_info.backendId = dst_id;
- tablet_commit_infos.emplace_back(std::move(commit_info));
+ if (!final_result_fanout) {
Review Comment:
[P1] Keep commit evidence when the sole owner response is lost
This suppresses every successful `(tablet, backend)` pair on a fanned-out
result, but delivery to the only unmarked owner is not acknowledged or retried.
In v2, a source that already reached quorum may end its bounded wait and close
its retained stream; if that source was the last closer, `_report_result`
merely logs the failed owner write while another source can receive this marked
success and then drop its commit info here. The v1 path has the same failure at
`vtablet_writer.cpp:1285`: a lost last-closer unary response invokes the
terminal failure callback while every deferred response is marked. Thus all
writers can return with no commit evidence for a replica that committed, and FE
can reject the load. Please let fanout recipients report the duplicate-safe
pair, or add acknowledged ownership with fallback, and test loss of only the
selected owner response in both protocols.
##########
be/src/load/channel/load_stream.cpp:
##########
@@ -759,27 +809,24 @@ void LoadStream::_dispatch(StreamId id, const
PStreamHeader& hdr, butil::IOBuf*
} break;
case PStreamHeader::CLOSE_LOAD: {
DBUG_EXECUTE_IF("LoadStream.close_load.block", DBUG_BLOCK);
- std::vector<int64_t> success_tablet_ids;
- FailedTablets failed_tablets;
- std::vector<PTabletID> tablets_to_commit(hdr.tablets().begin(),
hdr.tablets().end());
- bool all_closed =
- close(hdr.src_id(), tablets_to_commit, &success_tablet_ids,
&failed_tablets);
- _report_result(id, Status::OK(), success_tablet_ids, failed_tablets,
true);
- std::lock_guard<bthread::Mutex> lock_guard(_lock);
- // if incremental stream, we need to wait for all non-incremental
streams to be closed
- // before closing incremental streams. We need a fencing mechanism to
avoid use after closing
- // across different be.
- if (hdr.has_num_incremental_streams() && hdr.num_incremental_streams()
> 0) {
- _closing_stream_ids.push_back(id);
- } else {
+ auto result = _close_load(id, hdr);
+ if (result.report_current_stream) {
+ if (result.report_final_result_on_current_stream) {
+ _report_result(id, Status::OK(), result.success_tablet_ids,
result.failed_tablets,
+ true);
+ } else {
+ _report_result(id, Status::OK(), {}, {}, true);
+ }
+ }
+ for (const auto& stream : result.streams_to_report) {
Review Comment:
[P1] Avoid serially blocking every final-result recipient
Each `_report_result` here synchronously reaches `_write_stream`; on
`EAGAIN` that call can wait in `brpc::StreamWait` for
`load_stream_eagain_wait_seconds` (600 seconds by default). Because this
unordered loop must finish before any retained stream is closed, one
non-draining representative can keep every later healthy source from receiving
EOS or `StreamClose` even though tablet close has completed, causing unrelated
writers to hit their query timeout. Please dispatch/close each representative
independently (or otherwise bound per-recipient blocking), and add a
stalled-recipient test that proves another source completes promptly.
##########
fe/fe-core/src/main/java/org/apache/doris/transaction/DatabaseTransactionMgr.java:
##########
@@ -627,6 +634,21 @@ private void checkCommitStatus(List<Table> tableList,
TransactionState transacti
throw new
TransactionCommitFailedException("could not find replica for tablet ["
+ tabletId + "], backend [" +
tabletBackend + "]");
}
+ if (!crossAzSuccQuorum.isEmpty()) {
+
backendLocationTags.computeIfAbsent(tabletBackend, backendId -> {
+ Backend backend =
env.getCurrentSystemInfo().getBackend(backendId);
+ return backend == null ? "" :
backend.getLocationTag().value;
+ });
+ boolean canLoad =
env.getCurrentSystemInfo().checkBackendAlive(tabletBackend)
Review Comment:
[P1] Do not shrink the configured AZ floor after planning
This recomputes target eligibility from live heartbeat, badness, and replica
state, whereas the sink and both BE writers clamp against the destination set
serialized when the load was planned. For planned az1 targets A/B plus az2 C
under `az1:2`, A+C can satisfy the ordinary quorum; if failed B becomes
dead/bad/non-loadable before commit, it disappears from this denominator and
line 699 lowers the az1 requirement to one, so FE accepts fewer AZ successes
than the load was planned to require. The new dead/bad test marks replicas
before the transaction and therefore codifies only pre-planning exclusion.
Please retain the planned eligible destination denominator (or equivalent
transaction-bound allocation) and test an in-flight eligibility transition.
##########
fe/fe-core/src/main/java/org/apache/doris/transaction/DatabaseTransactionMgr.java:
##########
@@ -627,6 +634,21 @@ private void checkCommitStatus(List<Table> tableList,
TransactionState transacti
throw new
TransactionCommitFailedException("could not find replica for tablet ["
+ tabletId + "], backend [" +
tabletBackend + "]");
}
+ if (!crossAzSuccQuorum.isEmpty()) {
+
backendLocationTags.computeIfAbsent(tabletBackend, backendId -> {
+ Backend backend =
env.getCurrentSystemInfo().getBackend(backendId);
+ return backend == null ? "" :
backend.getLocationTag().value;
+ });
+ boolean canLoad =
env.getCurrentSystemInfo().checkBackendAlive(tabletBackend)
+ && !replica.isBad()
+ && (replica.getState().canLoad()
+ || (replica.getState() ==
Replica.ReplicaState.DECOMMISSION
+ && replica.getPostWatermarkTxnId() < 0
+ && replica.getLastFailedVersion() <
0));
+ if (canLoad) {
Review Comment:
[P1] Use the sink's row-binlog target set for this denominator
`OlapTableSink` sends a row-binlog write only to replicas whose
`(backend,pathHash)` is also present for the base tablet, but this loop counts
every live/good/loadable row-binlog replica. After ordinary independent repair,
the base may be on A/B/D while the binlog tablet is on A/B/C, with all replicas
NORMAL; the sink targets binlog A/B, so BE clamps `az2:2` to B alone, while FE
counts B+C here and rejects because untargeted C has no commit info. This needs
no policy/topology change during the load and is distinct from the existing
CLONE concern. Please validate against the serialized destination IDs or
reproduce the exact colocation intersection, with a repaired row-binlog
topology 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]
