morningman opened a new pull request, #69525:
URL: https://github.com/apache/doris/pull/69525
### What problem does this PR solve?
Issue Number: None
Related PR: #52971, #59990
Problem Summary:
**In short.** With batch edit log on (the default), a queued edit log entry
holds a reference to the live object, and the `EditLog-Flusher` thread
serializes it later without any lock. Since transaction persistence moved out
of the lock (#59990, `enable_txn_log_outside_lock`, also the default), other
threads keep changing the same `TransactionState` while its entry waits in the
queue. When such a change lands while the flusher iterates one of its maps, the
flusher gets a `ConcurrentModificationException`, treats it as a failed write
and exits the FE. This PR serializes every queued entry in the thread that
submits it, while that thread still holds the lock that guards the object,
makes the remaining lock-free writers of `TransactionState` take its monitor,
and stops the FE from serving for up to 300 seconds after such a failure.
**Background.**
- Batch edit log (#52971): `submitEdit` / `logEditWithQueue` put an
`EditLogItem(op, writable)` into a queue; `flushEditLog` drains it and calls
`JournalBatch.addJournal(op, writable)`, which is where the object is
serialized. The comment on `addJournal` says the data object is safe to change
once it returns, which holds for the thread that calls it, but for a queued
entry that thread is the flusher, at a time nobody controls.
- Outside-lock transaction persistence (#59990): commit, finish and abort
change the in-memory state and call `submitEdit` inside `synchronized
(transactionState)`, then release the monitor and `await()` the entry. The new
state (for example `COMMITTED`) is visible to other threads before the entry is
serialized.
- `PublishVersionDaemon` reads committed transactions without their monitor
(the code says so in `unprotectedCommitTransaction`) and writes
`tableIdToTabletDeltaRows` in `tryFinishOneTxn` (`HashMap.putAll`) every 10 ms
for every committed transaction, and clears it in `pruneAfterVisible`. The
version check of `finishTransaction` removes commit infos of dropped tables and
partitions outside the monitor. All of these maps are serialized fields of
`TransactionState`.
**The problem, and what it cost.**
- A P0 run of an unrelated PR (#69295, TeamCity build 1075936) lost its FE
at 17:40:26.930:
```
ERROR (EditLog-Flusher|77) [EditLog.flushEditLog():268] Fatal Error :
write stream Exception
java.util.ConcurrentModificationException
at java.util.HashMap$HashIterator.nextNode(HashMap.java:1597)
at java.util.HashMap$EntryIterator.next(HashMap.java:1630)
at
com.google.gson.internal.bind.MapTypeAdapterFactory$Adapter.write(MapTypeAdapterFactory.java:217)
...
at
org.apache.doris.transaction.TransactionState.write(TransactionState.java:781)
at org.apache.doris.journal.JournalEntity.write(JournalEntity.java:199)
at
org.apache.doris.journal.JournalBatch.addJournal(JournalBatch.java:64)
at org.apache.doris.persist.EditLog.flushEditLog(EditLog.java:231)
```
The iterated map is a direct `HashMap` field of `TransactionState`
(`idToTableCommitInfos`, `loadedTblIndexes` or `tableIdToTabletDeltaRows`). At
that moment two INSERT transactions (3956, 3957) still waited for their
`COMMITTED` entries (their threads were in `await()` in the jstack, and never
returned), yet `PublishVersionDaemon` had already sent their publish tasks
(26.907) and run `tryFinishOneTxn` on them, which starts with
`setTableIdToTabletDeltaRows` (26.918, 26.928).
- `System.exit(-1)` from the flusher runs the shutdown hook, and
`gracefulShutdown` waits up to 300 seconds for running queries. The FE kept
accepting connections for six minutes with a dead journal: every thread that
writes the edit log stayed blocked in `await()` with its catalog, database or
table locks held, so DDL failed with `Failed to acquire catalog lock`, loads
hung, and then 5180 cases failed with `Connection refused` (5187 failed, 986
passed).
- Even without an exception, the journal gets the state of the moment the
flusher reaches the object, not the one the producer committed: a `COMMITTED`
entry can carry publish-time delta rows or a later status.
- The race window is tiny, so the crash is rare; but it exists with the
default configuration, in production as well as in CI.
**How this PR fixes it.**
1. Serialize queued entries in the submitting thread.
`JournalBatch.serialize(op, data)` produces the entity bytes and
`JournalBatch.addEntity` adds them to a batch (`addJournal` is now the two in a
row). `submitEdit` and `logEditWithQueue` serialize before they queue the
entry, and `EditLogItem` carries the bytes instead of the object, so the
flusher never reads an object that may still change. The bytes written to BDBJE
do not change, so replay and compatibility are not affected. This covers every
queued operation, not only transactions; `MTMV` and table stream cleanup submit
entries the same way. A serialization failure exits the FE like any other write
failure, because the in-memory state has already changed.
2. Make the remaining lock-free writers of `TransactionState` take its
monitor: `setTableIdToTabletDeltaRows` and `pruneAfterVisible` are
`synchronized`, and `finishCheckPartitionVersion` /
`finishCheckPartitionVersionWithSubTxns` run their removals under `synchronized
(transactionState)` (the bodies moved unchanged into `unprotected...` methods).
With (1), commit and finish serialize under the same monitor, so these writes
cannot overlap the serialization.
3. Do not keep serving after the edit log fails. Every write failure path of
`EditLog` (flush, direct write, serialization, interrupted wait) goes through
`exitOnWriteFailure`, which records the failure before `System.exit(-1)`.
`DorisFE.gracefulShutdown` skips the wait for running queries when the edit log
can not be written.
4. Unit tests (below).
**Results.**
| Scenario | Before | After |
|---|---|---|
| Commit entry waits in the queue while the publish of the same transaction
updates its delta rows | the flusher may throw
`ConcurrentModificationException` and the FE exits | the entry was serialized
at submission; the flusher only writes bytes |
| What the journal records for a queued entry | the object as the flusher
finds it later (`testSubmitEditWritesStateAtSubmission` reads back `VISIBLE`
with delta rows on master) | the state at submission (`COMMITTED`, no delta
rows) |
| `setTableIdToTabletDeltaRows`, `pruneAfterVisible`, removal of dropped
tables in the version check | run without the monitor | wait for the monitor
(`testPublishUpdatesWaitForMonitor`,
`testFinishCheckPartitionVersionWaitsForMonitor`) |
| FE exits because the edit log can not be written, with queries running |
the shutdown hook waits up to 300 s while the FE keeps serving | the hook
returns at once (`testGracefulShutdownDoesNotWaitAfterEditLogWriteFailure`) |
**Classes, and how they call each other.**
- `JournalBatch`: `serialize` turns an operation into entity bytes;
`addEntity` adds those bytes to a batch.
- `EditLog`: `submitEdit` / `logEditWithQueue` serialize, then queue
`EditLogItem(op, entity)`; `flushEditLog` appends the entities and writes the
batch; `exitOnWriteFailure` / `isWriteFailed` record a write failure.
- `TransactionState`: the two publish-side writers take the monitor.
- `DatabaseTransactionMgr`: the version checks of finish take the monitor.
- `DorisFE`: `gracefulShutdown` returns at once after a write failure.
```
commit / finish / abort PublishVersionDaemon
synchronized (transactionState) { tryFinishOneTxn
change in-memory state
setTableIdToTabletDeltaRows (synchronized)
EditLog.submitEdit tryFinishTxnSync
JournalBatch.serialize <-- bytes taken pruneAfterVisible
(synchronized)
queue EditLogItem(op, entity)
}
item.await() EditLog-Flusher
flushEditLog
JournalBatch.addEntity(entity)
Journal.write(batch)
on failure:
exitOnWriteFailure -> System.exit
->
DorisFE.gracefulShutdown skips the wait
```
Not changed: the journal format, the order of entries,
`enable_batch_editlog` / `enable_txn_log_outside_lock`, and the replay paths.
### Release note
Fix a race in which the master FE could exit with `Fatal Error : write
stream Exception` (`ConcurrentModificationException`) while writing a
transaction to the edit log. After an edit log write failure, the FE now exits
without waiting for running queries.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- `run-fe-ut.sh` on this branch: 116 tests, 0 failures: `EditLogTest`
(8, two new), `TransactionStateTest` (5, two new), `DorisFETest` (1, new),
every test class in `org.apache.doris.transaction` (including
`DatabaseTransactionMgrTest` 35 and `GlobalTransactionMgrTest` 15),
`BDBJEJournalTest` (3, writes `JournalBatch` to a real BDBJE), and
`TableStreamManagerCleanupTest` (9) / `StreamingInsertJobSchemaPersistenceTest`
(14), which submit edits through `EditLogItem`. `mvn checkstyle:check -pl
fe-core`: 0 violations.
- The new tests fail without the fix. With the main code of this PR
reverted, both `EditLogTest` cases read back `VISIBLE` instead of `COMMITTED`,
and both `TransactionStateTest` cases report that the update did not wait for
the monitor. With only the new check in `gracefulShutdown` removed,
`DorisFETest` times out after 10 s.
- No regression test: the crash needs a write to land while the flusher
iterates a map, which a regression case can not arrange. The unit tests park
the flusher in a fake journal to make the ordering deterministic.
- Behavior changed:
- [ ] No.
- [x] Yes. Queued edit log entries are serialized in the submitting
thread instead of the flusher, so the journal records the state at submission.
After an edit log write failure the shutdown hook no longer waits for running
queries.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
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]
