deardeng commented on code in PR #67621:
URL: https://github.com/apache/doris/pull/67621#discussion_r4022001446
##########
gensrc/thrift/MasterService.thrift:
##########
@@ -123,6 +140,12 @@ struct TReportRequest {
15: optional list<AgentService.TIndexPolicy> index_policy
// Running query/loading tasks
16: optional i64 running_tasks
+ // Top-N tablets by query scan count on this BE since the previous report.
+ // Entries set only tablet_id / scan_count_delta / last_query_time_ms /
delta_window_ms.
+ 17: optional list<TActiveTabletStat> top_query_tablets
Review Comment:
Confirmed for the BE-first direction, and the commit message's claim that
"upgrade order does not matter" is wrong — it matters, and one of the two
orders destroys data.
BE-first: the new BE fills 17/18, the old FE skips the unknown fields and
still answers OK, the BE reads OK as delivery and commits. Activity is
discarded for the whole length of the rolling upgrade, silently — no warning,
no metric. FE-first is benign by comparison: `isSet` is false, that backend
simply has no entry, and nothing is destroyed.
Not fixing it in this PR, but the fix is an acknowledgement rather than a
version fence. Add `optional bool active_tablets_accepted` to `TMasterResult`
and gate `active.commit()` on it. An old FE leaves it unset, the BE does not
commit, deltas keep accumulating and are delivered in full once FE is upgraded.
That is self-healing, and it reuses the existing "report not delivered" path
rather than introducing a new state machine — the same path already covers a
failed `handle_report` and an exhausted retry loop.
The reason it is not in this change: it alters the commit contract, and the
behaviour against a permanently old FE (unbounded delta accumulation, and
`accessCount` growing without bound even though `delta / delta_window_ms` stays
correct) has to be bounded before it ships. It also composes with the failover
handshake discussed on the other thread, so the two are better designed
together than bolted on separately.
Until then the mitigation is operational: upgrade FE first, or set
`enable_active_tablet_sliding_window_access_stats = false` for the duration of
the upgrade. That one is `mutable`, and with it off `hasActiveStats` is false,
so cold-first selection is disabled and the scheduler falls back to its
pre-feature behaviour. Note that
`enable_cloud_active_tablet_priority_scheduling` is not mutable and cannot be
used for this.
--
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]
