FreeOnePlus opened a new pull request, #68429:
URL: https://github.com/apache/doris/pull/68429
# [fix](storage) Preserve pending publish retries for unfinished MoW writes
### What problem does this PR solve?
Related issue: #62057. This PR does **not** claim to reproduce or close that
issue's specific empty-success case.
**Draft / validation status:** source changes and tests are implemented, but
a full BE build and runtime validation have **not** completed. Do not merge or
deploy based on the auxiliary workflow's green status: it records the build
attempt and publishes the branch even when the attempt cannot complete.
#### Problem Summary
In the non-cloud MoW write path, a replica can receive `PUBLISH_VERSION`
while its local transaction is still PREPARED and its rowset has not been
committed. The existing null-rowset path reports the tablet as failed, but does
not retain this request for local asynchronous recovery.
Separately, `_process_async_publish()` removes an entry and its durable
pending marker immediately after submitting work to the tablet publish pool.
Submission may fail, and the worker may return without publishing because local
transaction state is unavailable, migration locking fails, or publishing fails.
In these cases this local recovery path has already discarded its retry intent;
other reporting, re-publishing or repair mechanisms may still recover the
replica.
This change preserves a bounded local retry opportunity. It is a recovery
fix, not a claim that Delete Bitmap computation is faster, that a 3.x-to-4.x
regression has been proven, or that all query slowdowns are resolved.
#### Changes
- Retain the requested version when normal publish encounters a PREPARED MoW
tablet with a null local rowset. Continue reporting the original tablet
failure; do not fabricate success or change FE quorum handling.
- Keep pending requests and their existing durable markers while an attempt
is queued or running. Remove the marker and then the in-memory request only
after the target version is confirmed present. Preserve the existing
bounded-backlog pruning policy, including a warning for pruned entries.
- Track one asynchronous attempt per tablet at a time. Record
submission/execution results using release/acquire completion publication and
retry completed failures with a one-second monotonic backoff instead of
re-submitting every producer tick.
- Add an asynchronous-path readiness getter requiring a committed, non-null
rowset and, for MoW, installed Delete Bitmap context. This avoids using the
interval between `commit_txn()` and `set_txn_related_delete_bitmap()` as
publish readiness. It does not refactor normal commit into a new atomic
protocol.
- Preserve binlog-first migration/rowset-update lock order; recheck the
target version and its immediate predecessor before asynchronous publish, and
reject shutdown tablets.
- Propagate pending-marker persistence and pool-submission errors instead of
silently discarding them.
- Keep failed requests retryable **without indefinitely fencing Clone**:
`get_pending_publish_min_version()` excludes completed failed attempts from
Clone's repair ceiling. Queued/running attempts remain fenced. No FE repair
scheduler policy is changed.
#### Tests added (not yet executed)
Eleven `AsyncPublishRecoveryTest` cases cover:
1. Missing transaction retention.
2. Prepared-to-committed retry.
3. Incomplete MoW context.
4. Duplicate queued registration.
5. Pool rejection.
6. Predecessor ordering.
7. Durable-marker replay after a failed attempt.
8. Already-published idempotency.
9. Publish-before-local-commit registration.
10. Aborted transaction rejection.
11. Failed-attempt/Clone-ceiling coexistence.
One additional `TxnManagerTest` covers committed rowset/MoW readiness and
Tablet UID mismatch. The existing async-publish storage-engine unit test is
adapted to retained-intent semantics.
A three-replica Docker fault-injection regression delays one replica's local
commit, injects asynchronous failures, and then checks that it catches up
without an additional write or replica cloning. It uses no production data.
The marker-replay unit case exercises the recovery interface; it is **not**
a real process-restart test. Full restart, Clone concurrency, row-binlog and
non-empty update workloads remain validation requirements.
#### Validation performed
Passed on the isolated fork workflow:
- Repository `build-support/clang-format.sh`, clang-format **16.0.6**, and
changed-file dry-run checks.
- `git diff --check`.
- `build-support/check-build-hygiene.sh`: header layering/reach,
extern-template pairing, unity-skip coverage.
Attempted with JDK 17:
```bash
bash run-be-ut.sh --run \
--filter='AsyncPublishRecoveryTest.*:StorageEngineTest.TestAsyncPublish:TxnManagerTest.PublishableInfo*'
-j 2
```
The hosted-runner attempt was bounded to 150 seconds and exited **124 while
initializing third-party submodules**, before tests executed. This is not
evidence of a source compilation failure or of a successful build. Full BE
compilation, ASAN/TSAN, clang-tidy, Docker regression and comparative
performance replay are unverified.
Auxiliary logs/artifacts:
https://github.com/FreeOnePlus/doris/actions/runs/35825091270
The auxiliary workflow and encoded patches are on a separate diagnostics
branch and are **not included** in this PR.
#### Source-review checkpoints and remaining gates
| Checkpoint | Current conclusion |
|---|---|
| Scope | Local non-cloud async-publish recovery only; no performance
improvement numbers claimed. |
| Concurrency | Producer owns request/attempt transitions under
`_async_publish_lock`; worker result uses release/acquire completion. Stress
testing remains pending. |
| Lock order | Existing binlog-first migration and rowset-update ordering
preserved. Pending-marker I/O is serialized under the async lock; contention
must be measured. |
| Lifecycle | Requests retain an attempt until completion; the worker owns a
separate shared pointer. Shutdown, replacement and real-restart integration
tests remain pending. |
| Correctness | Existing publish helper/quorum/version checks retained, with
readiness and predecessor guards. Full runtime correctness remains unverified. |
| Errors | Pool submission and marker persistence failures are retained or
propagated. True failure is not reported as publish success. |
| Recovery/Clone | Completed failed attempts do not cap Clone's repair
range; a dedicated unit test is added, while live Clone concurrency still needs
testing. |
| Compatibility | No new wire/storage format or public configuration. This
is based on master; a 4.0.8 backport has not been produced. |
| Parallel paths | Normal publish receives a narrow prepared-rowset handoff;
its commit registration protocol is not otherwise refactored. Cloud behavior is
out of scope. |
| Observability | Retry and bounded-pruning logs identify tablet/version;
additional operational metrics may be desirable after runtime review. |
| Tests/results | Twelve new BE unit cases and one Docker regression added;
no fabricated `.out` or passing-runtime claim. |
Before marking ready for review, validate repeated requests, normal-vs-async
publish races, cancellation, restart, Clone/replacement, attached row-binlog
failures, and non-empty MoW updates. Measure replica catch-up time, actual
Clone bytes, pending backlog, write throughput and uncached query latency under
comparable load.
This PR does not optimize Delete Bitmap algorithms, change the all-writers
Close barrier, disable repair, or change Compaction scheduling.
### Release note
Retain pending local publish requests across unfinished MoW writes and
transient asynchronous failures so eligible replicas can retry publishing local
rowsets without losing the retry intent. Failed local attempts do not
indefinitely prevent necessary replica repair.
### Check List (For Author)
- Test
- [ ] Regression test — added; execution pending
- [ ] Unit Test — added; full build and execution pending
- [x] Manual test — formatting, diff and build-hygiene checks only,
documented above
- [ ] No need to test or manual test
- Behavior changed:
- [ ] No.
- [x] Yes. Pending requests survive queued/running and failed attempts;
completed failures are retried with backoff and excluded from Clone's
pending-version ceiling.
- Does this need documentation?
- [x] No. No new public configuration or wire/storage format.
- [ ] Yes.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases and actual test results
- [ ] Confirm document
- [ ] Add branch pick label only after backport validation
--
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]
