Doris-Breakwater commented on issue #66051:
URL: https://github.com/apache/doris/issues/66051#issuecomment-5081414105
Breakwater-GitHub-Analysis-Slot: slot_a77e50f6a370
## Initial maintainer assessment
**Current judgment: the reported data-loss chain is not established, and the
two proposed fixes do not follow from the current code.** The issue points at
safety-critical code and should receive a deterministic reproducer or incident
evidence, but source inspection contradicts both asserted defects. I would not
mark the listed branches as confirmed affected or merge the proposed changes on
the present evidence.
The issue is open and currently has no labels, assignee, type, milestone, or
linked development. I inspected the cited code on the live branch heads:
- `branch-2.1`: `a3052de2e5cbc397d67b9cd9d5cd9701aaa6426c`
- `branch-3.1`: `7f5ba43de6c7f148e1dacdb7cb838394d2ec1537`
- `branch-4.1`: `1ac74c857454c7dc59005fdc688eeeb4ad58cf75`
- `master`: `b9c561a03849a78d3cb41294cce8c0ae610257a0`
### Why the proposed root cause does not hold
1. **A non-cooldown full clone preserves a consistent pair: clone-source
remote rowsets plus clone-source `cooldown_meta_id`.**
A full snapshot copies the source tablet metadata, explicitly selects all
remote rowsets, and records those selected rowset metas in the snapshot. During
rowset-ID conversion, remote rowset metadata and IDs are copied unchanged; only
local rowsets receive new IDs. `_finish_full_clone()` then installs those
cloned rowsets and, for a non-cooldown destination, installs the same source
`cooldown_meta_id`.
Therefore, the `else` branch does not by itself create the issue's
required state of “same `cooldown_meta_id`, different remote rowset IDs.” It
keeps the identifier and the remote rowset set from the same snapshot together.
See
[`SnapshotManager`](https://github.com/apache/doris/blob/b9c561a03849a78d3cb41294cce8c0ae610257a0/be/src/storage/snapshot/snapshot_manager.cpp#L642-L766),
[`convert_rowset_ids`](https://github.com/apache/doris/blob/b9c561a03849a78d3cb41294cce8c0ae610257a0/be/src/storage/snapshot/snapshot_manager.cpp#L177-L242),
and
[`_finish_full_clone`](https://github.com/apache/doris/blob/b9c561a03849a78d3cb41294cce8c0ae610257a0/be/src/storage/task/engine_clone_task.cpp#L962-L1035).
The special-case fresh ID for the **current cooldown replica** was
deliberately introduced by
[#17644](https://github.com/apache/doris/pull/17644): that replica owns the
current term's remote `.meta` path, so replacing its rowsets from another
replica while reusing the source ID could make its local state disagree with
the already-written remote meta. That is the exact scenario described by the
in-code comment. A non-cooldown replica does not own/write that term meta and
will follow the owner when its ID differs.
Changing every non-cooldown clone to a fresh ID would deliberately create
an ID mismatch and force an extra follower synchronization. It does **not**
force the cooldown replica to rewrite remote meta, contrary to the proposed
explanation.
2. **The FE request-side null guard already exists on all four cited
branches.**
`confirmUnusedRemoteFiles()` starts each entry with:
```java
if (!info.isSetCooldownMetaId()) {
LOG.warn("cooldown_meta_id is null");
return;
}
```
The BE collector also only processes tablets with an initialized ID and
explicitly sets the request field. For a replica whose `getCooldownMetaId()` is
null, `info.cooldown_meta_id.equals(null)` is false, so the existing negated
condition returns without confirming deletion. The suggested explicit
replica-null test may improve readability, but it does not change the decision.
See
[`confirmUnusedRemoteFiles`](https://github.com/apache/doris/blob/b9c561a03849a78d3cb41294cce8c0ae610257a0/fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java#L401-L483).
3. **The deletion path is fail-closed for the state described in the issue.**
The current cooldown replica collects its live remote rowset IDs and its
`cooldown_meta_id` under the tablet header lock. FE confirms only when the
configured cooldown replica matches, there are enough live replicas, every
replica reports the current cooldown term, and every reported meta ID equals
the request ID. Any missing or different ID blocks deletion. See
[`collect_tablet_unused_remote_files`](https://github.com/apache/doris/blob/b9c561a03849a78d3cb41294cce8c0ae610257a0/be/src/storage/olap_server.cpp#L1320-L1364).
Consequently, differing `CooldownMetaId` values are a
synchronization-in-progress signal that prevents reclamation, not evidence that
deletion has been approved. An empty ID is also not sufficient evidence of loss
(for example, a tablet may not yet have cooled data), and it prevents
confirmation rather than permitting it.
### Missing evidence needed to validate a real bug
The reproduction section is currently an outline, not an executed
reproducer. The decisive proof would be a non-cooldown full clone producing
**two replicas with the same non-null `cooldown_meta_id` but different remote
`rowset_id_v2` lists**. Please provide:
- Exact Doris commit/version, table and storage-policy DDL, replica count,
and the exact command/fault used to force a **full** clone.
- Tablet ID plus the clone source replica, destination replica, current
cooldown replica, and cooldown term.
- A timeline before clone, immediately after clone, after tablet report, at
FE confirmation, and after deletion. At each point include every replica's
`CooldownReplicaId`, `CooldownTerm`, `CooldownMetaId`, and remote rowset IDs.
- BE logs covering `begin to finish full clone`, cooldown-meta write/follow
activity, `begin to confirm unused remote files`, and every `delete unused
file` line; FE logs covering the corresponding tablet report and
`confirmUnusedRemoteFiles` decision.
- The current term's remote `.meta` decoded to its
`rs_metas[*].rowset_id_v2`, an object listing or object-store audit event
proving the exact delete, and the subsequent query's full `[NOT_FOUND]` error.
- If this came from an incident, the relevant tablet headers/metas before
any repair or restart. Bucket version history is especially useful to
distinguish Doris deletion from lifecycle rules or an external deleter.
### Recommended next steps
1. Keep this as **needs reproduction / needs information**, not confirmed
data loss. Apply the repository's applicable triage label; none is currently
present.
2. Add a focused BE test that full-clones both (a) a non-cooldown
destination and (b) the current cooldown destination, asserting the
`(cooldown_meta_id, remote rowset-ID list)` invariant in each case.
3. Add an FE unit test for unset request IDs and null replica IDs to
document that both paths reject confirmation.
4. If a deterministic reproducer demonstrates the required
same-ID/different-rowset state, investigate the first operation that broke that
invariant; changing clone ID generation alone should not be assumed to be the
correct fix.
5. For an active incident only, preserve object versions/audit logs and
pause or substantially delay remote-file reclamation while collecting evidence.
The broad topology freeze proposed in the issue is not justified as a general
mitigation until the causal chain is reproduced.
No code changes were made during this assessment.
--
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]
