Doris-Breakwater commented on issue #66301:
URL: https://github.com/apache/doris/issues/66301#issuecomment-5130069301

   Breakwater-GitHub-Analysis-Slot: slot_8aa6e21226df
   
   ## Initial maintainer triage
   
   **Judgment:** this is a credible, high-impact storage/recovery report, but 
the supplied evidence proves a version-graph hole, not yet that a plain BE 
`SIGKILL` tore a publish metadata update. The issue currently has no labels; 
`area/storage`, `area/tablet-repair`, and `area/tablet-report` look appropriate.
   
   ### What the 4.1.0-rc03 source confirms
   
   * The reported error is emitted only when the BE cannot find a graph path 
covering the requested range. It confirms a discontinuous set of visible rowset 
versions, but does not identify which operation lost the rowset: 
[`VersionGraph::capture_consistent_versions`](https://github.com/apache/doris/blob/4.1.0-rc03/be/src/storage/version_graph.cpp#L417-L467).
   * Publish does **not** atomically replace a whole tablet header. It first 
changes the rowset to `VISIBLE` in memory, later persists that individual 
rowset meta with a RocksDB `Put`, removes the transaction from the in-memory 
txn map, and then adds the visible rowset to the tablet: 
[`TxnManager::publish_txn`](https://github.com/apache/doris/blob/4.1.0-rc03/be/src/storage/txn/txn_manager.cpp#L518-L622),
 
[`publish_version_and_add_rowset`](https://github.com/apache/doris/blob/4.1.0-rc03/be/src/storage/task/engine_publish_version_task.cpp#L423-L463).
   * The normal restart path is intended to make either side of that boundary 
recoverable: persisted `COMMITTED` rowsets are restored to `TxnManager`, while 
persisted `VISIBLE` rowsets are added back to the tablet 
([`DataDir::load`](https://github.com/apache/doris/blob/4.1.0-rc03/be/src/storage/data_dir.cpp#L520-L607)).
 There is also a regression test showing FE report/resend repairs failures 
injected both before and after `make_visible` 
([test](https://github.com/apache/doris/blob/4.1.0-rc03/regression-test/suites/insert_p0/test_be_inject_publish_txn_fail.groovy#L36-L61)).
 Therefore, an isolated publish interrupted by a process-only `SIGKILL` should 
not by itself be assumed to produce a permanent hole.
   * There is nevertheless a concrete risky ordering in this release. Publish 
work uses concurrent tokens, and the per-tablet `rowset_update_lock` plus 
continuity gate are applied only to unique-key merge-on-write tablets 
([submission](https://github.com/apache/doris/blob/4.1.0-rc03/be/src/storage/task/engine_publish_version_task.cpp#L132-L133),
 [continuity 
gate](https://github.com/apache/doris/blob/4.1.0-rc03/be/src/storage/task/engine_publish_version_task.cpp#L194-L239),
 
[lock](https://github.com/apache/doris/blob/4.1.0-rc03/be/src/storage/task/engine_publish_version_task.cpp#L466-L483)).
 For a non-MoW tablet, version `N+1` can become persistent while version `N` is 
still before its rowset-meta write. Killing the BE in that state can reload 
`N+1` as visible and `N` as committed, which is exactly the shape needed for a 
graph hole until `N` is republished.
   * Metadata writes use `sync_tablet_meta`, whose default is `false`, as the 
RocksDB `WriteOptions.sync` value 
([config](https://github.com/apache/doris/blob/4.1.0-rc03/be/src/common/config.cpp#L750),
 
[write](https://github.com/apache/doris/blob/4.1.0-rc03/be/src/storage/olap_meta.cpp#L156-L172)).
 Loss of acknowledged recent metadata is consequently much more plausible for 
an actual node/storage failure than for an OOM-killed container while the node 
and volume remain healthy. Those two triggers need to be separated.
   * A committed missing rowset is not necessarily terminal. After 
`pending_data_expire_time_sec` (default 1800 seconds), BE reports the retained 
transaction, and FE can resend a visible transaction's publish task 
([BE](https://github.com/apache/doris/blob/4.1.0-rc03/be/src/storage/txn/txn_manager.cpp#L873-L895),
 
[FE](https://github.com/apache/doris/blob/4.1.0-rc03/fe/fe-core/src/main/java/org/apache/doris/catalog/LocalTabletInvertedIndex.java#L495-L553)).
 It becomes unrecoverable through that route if the missing rowset meta/data is 
absent or FE has already discarded the transaction.
   
   The same essential publish locking and `sync_tablet_meta=false` behavior is 
still present on current `master`; I did not find a later fix that clearly 
closes this case.
   
   ### `ADMIN SET REPLICA VERSION` behavior
   
   The command only journals and changes the FE `Replica` object; it has no BE 
RPC and cannot rewrite rowset metadata ([FE 
implementation](https://github.com/apache/doris/blob/4.1.0-rc03/fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java#L6981-L7025)).
 BE full reports compute and report the highest **continuous** version and, 
after the same gap persists for 60 seconds, set `version_miss` ([BE 
report](https://github.com/apache/doris/blob/4.1.0-rc03/be/src/storage/tablet/tablet.cpp#L1587-L1625),
 [reported 
version](https://github.com/apache/doris/blob/4.1.0-rc03/be/src/storage/tablet/tablet.cpp#L1690-L1707)).
   
   The statement that the next report always “reverts” the admin value needs 
the exact SQL and before/after replica fields. FE syncs a report when the BE's 
continuous version is greater than the FE version; it does not normally lower a 
higher FE version from a report 
([`needSync`](https://github.com/apache/doris/blob/4.1.0-rc03/fe/fe-core/src/main/java/org/apache/doris/catalog/LocalTabletInvertedIndex.java#L574-L609)).
 If the command lowered the FE value below the BE's continuous prefix, 
advancing it again is expected. In all cases, this command is metadata 
bookkeeping, not an on-disk repair mechanism; its user-facing 
documentation/error message should say so.
   
   ### Existing emergency recovery paths
   
   The “only repair is a full rebuild” conclusion is not correct for 4.1.0-rc03:
   
   1. `SET skip_missing_version = true` is explicitly implemented for 
emergency, incomplete reads when all replicas have missing versions. It skips 
missing data and is intended only for temporary recovery/export ([session 
variable documentation in 
source](https://github.com/apache/doris/blob/4.1.0-rc03/fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java#L2086-L2098)).
   2. The authenticated `POST 
/api/pad_rowset?tablet_id=...&start_version=...&end_version=...` BE action 
creates an empty visible rowset for a missing range and saves the tablet meta 
([implementation](https://github.com/apache/doris/blob/4.1.0-rc03/be/src/service/http/action/pad_rowset_action.cpp#L77-L124),
 [registered 
route](https://github.com/apache/doris/blob/4.1.0-rc03/be/src/service/http_service.cpp#L427-L429),
 [unit 
test](https://github.com/apache/doris/blob/4.1.0-rc03/be/test/storage/tablet/tablet_test.cpp#L272-L304)).
 It was introduced specifically for the case where every replica is broken.
   
   `pad_rowset` knowingly substitutes empty data for the missing versions. It 
should be used only with explicit data-loss acceptance, after taking a storage 
snapshot and proving that the original rowset meta/files cannot be recovered. 
It is currently too easy to misuse and poorly surfaced, but it already provides 
the BE-side salvage primitive requested by this issue.
   
   ### Information required to confirm this incident
   
   Please attach:
   
   * `SHOW CREATE TABLE` for an affected table, especially key type and 
`enable_unique_key_merge_on_write`, plus the exact `ADMIN SET REPLICA VERSION` 
statement and `SHOW TABLET <tablet_id>` / replica-status output immediately 
before and after the next full report.
   * The affected tablet ID and authenticated outputs of 
`/api/meta/header/<tablet_id>` and `/api/compaction/show?tablet_id=<tablet_id>` 
after restart. Preserve the PVC or a snapshot before attempting `pad_rowset`.
   * BE INFO/WARNING logs from at least 10 minutes before termination through 
startup and 35 minutes after startup, filtered in addition to the full files 
for the tablet ID, transaction IDs, `publish txn`, `add committed rowset`, `add 
visible rowset`, `expired txn`, `version_miss`, RocksDB/WAL, and I/O errors.
   * FE logs for the same interval, including publish-task completion/resend, 
tablet report, missing-version, clone/recovery, and the admin command.
   * Kubernetes `lastState.terminated`, events, exit code/signal, and node 
status sufficient to distinguish container OOM `SIGKILL` from node loss/reboot. 
Also provide the Ceph volume type/mount details and the effective BE value of 
`sync_tablet_meta`.
   * The exact missing version range(s), whether a corresponding `COMMITTED` 
rowset meta remains after restart, and whether its segment files still exist.
   
   ### Recommended maintainer next steps
   
   1. Add a selective fault point for one tablet/version after `make_visible` 
and before `RowsetMetaManager::save`. Persist `N+1`, stop the process while `N` 
is paused, restart, and test both non-MoW and MoW tables. Run separate variants 
for process-only `SIGKILL` and simulated node/storage durability loss, with 
`sync_tablet_meta` both false and true.
   2. Assert restart republish behavior before and after the 1800-second 
retained-transaction threshold; also cover the case where FE transaction state 
has been GC'd.
   3. If reproduced, prevent a higher version from becoming durably/reportably 
visible before the preceding version is durable (for all key models), or add a 
durable per-tablet publish journal/recovery marker. A durability boundary is 
also needed before FE can safely treat publish as complete under node failure.
   4. Surface `version_miss` and the exact missing ranges in operator-facing 
status, document `skip_missing_version` and a guarded `pad_rowset` recovery 
workflow, and clarify that `ADMIN SET REPLICA VERSION` never repairs BE storage.
   
   This should remain open as **needs reproduction / probable BE storage 
recovery bug**. The code contains a plausible ordering/durability mechanism, 
but the current report lacks the artifacts needed to distinguish it from a lost 
rowset meta, storage-layer durability failure, migration/compaction race, or an 
unreplayed retained transaction.
   


-- 
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]

Reply via email to