github-actions[bot] commented on code in PR #67053:
URL: https://github.com/apache/doris/pull/67053#discussion_r3841438594
##########
be/src/exec/pipeline/pipeline_fragment_context.cpp:
##########
@@ -2544,6 +2553,22 @@ void
PipelineFragmentContext::_coordinator_callback(const ReportStatusRequest& r
return;
}
+ if (req.report_query_statistics) {
+ // The client-facing FE owns the audit event and may differ from the
coordinator FE.
+ // Keep statistics in a separate RPC so they cannot invalidate the
terminal status payload.
+ Status statistics_status =
Review Comment:
Send the essential coordinator terminal report before attempting this
best-effort statistics RPC, or isolate statistics delivery behind a short
bounded async path. As written, a stalled `current_connect_fe` can consume one
or two 60-second Thrift waits here before a healthy `coord_addr` receives
commit/error metadata, and external-file ownership is not acknowledged until
afterward. This is distinct from the old global-reporter thread: it blocks this
query's essential terminal callback at the new split-RPC location. Please add a
test with slow audit FE A and healthy coordinator FE B.
##########
fe/fe-core/src/main/java/org/apache/doris/resource/workloadschedpolicy/WorkloadRuntimeStatusMgr.java:
##########
@@ -186,20 +186,40 @@ public void
updateBeQueryStats(TReportWorkloadRuntimeStatusParams params) {
return;
}
long beId = params.backend_id;
- // NOTE(wb) one be sends update request one by one,
- // so there is no need a global lock for beToQueryStatsMap here,
- // just keep one be's put/remove/get is atomic operation is enough
long currentTime = System.currentTimeMillis();
- BeReportInfo beReportInfo = beToQueryStatsMap.get(beId);
- if (beReportInfo == null) {
- beReportInfo = new BeReportInfo(currentTime);
- beToQueryStatsMap.put(beId, beReportInfo);
- } else {
- beReportInfo.beLastReportTime = currentTime;
- }
+ BeReportInfo beReportInfo = beToQueryStatsMap.computeIfAbsent(beId,
+ ignored -> new BeReportInfo(currentTime));
+ beReportInfo.beLastReportTime = currentTime;
for (Map.Entry<String, TQueryStatisticsResult> entry :
params.query_statistics_result_map.entrySet()) {
- beReportInfo.queryStatsMap.put(entry.getKey(),
Pair.of(currentTime, entry.getValue()));
+ Pair<Long, TQueryStatisticsResult> incoming = Pair.of(currentTime,
entry.getValue());
+ beReportInfo.queryStatsMap.compute(entry.getKey(), (queryId,
previous) ->
+ previous == null ||
shouldReplaceQueryStatistics(previous.second, incoming.second)
+ ? incoming : previous);
+ }
+ }
+
+ private boolean shouldReplaceQueryStatistics(TQueryStatisticsResult
previous,
+ TQueryStatisticsResult incoming) {
+ long previousGeneration = previous.isSetQueryStatisticsGeneration()
Review Comment:
This comparator only protects an upgraded FE. During the repository's
supported BE-then-FE rolling window, the old FE ignores these optional fields
and still unconditionally replaces the entry, so delayed periodic S0 can
overwrite acknowledged final S1 after the BE unregisters its retry state. This
is a distinct mixed-version instance not handled by the upgraded-FE ordering
fix. Please make final publication safe on BE independently of FE support, or
negotiate capability and retain/reconcile state for legacy destinations; add a
mixed-version test.
##########
fe/fe-core/src/main/java/org/apache/doris/resource/workloadschedpolicy/WorkloadRuntimeStatusMgr.java:
##########
@@ -186,20 +186,40 @@ public void
updateBeQueryStats(TReportWorkloadRuntimeStatusParams params) {
return;
}
long beId = params.backend_id;
- // NOTE(wb) one be sends update request one by one,
- // so there is no need a global lock for beToQueryStatsMap here,
- // just keep one be's put/remove/get is atomic operation is enough
long currentTime = System.currentTimeMillis();
- BeReportInfo beReportInfo = beToQueryStatsMap.get(beId);
- if (beReportInfo == null) {
- beReportInfo = new BeReportInfo(currentTime);
- beToQueryStatsMap.put(beId, beReportInfo);
- } else {
- beReportInfo.beLastReportTime = currentTime;
- }
+ BeReportInfo beReportInfo = beToQueryStatsMap.computeIfAbsent(beId,
Review Comment:
The timeout cleaner can classify this retained `BeReportInfo` as stale, then
this final update can refresh it and receive OK, and the cleaner can still
unconditionally remove the same object. The class comment previously tolerated
that race because a later periodic report healed it, but the BE now unregisters
immediately after this acknowledgement. Please make update and expiry atomic or
conditional per backend and query, and add a latch-controlled
cleanup-versus-final-update 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]
