kerneltime opened a new pull request, #10953: URL: https://github.com/apache/ozone/pull/10953
## What changes were proposed in this pull request? Two writers reach `TRANSACTION_INFO_KEY`, and only one of them is ordered against the data it describes. The double buffer writes the key *inside* its batch, so the transactions and the index describing them become visible together (`OzoneManagerDoubleBuffer.java:375-376`, committed at `:379-381`). It advances the state machine's applied index only afterwards, at `:396`. `takeSnapshotImpl` writes the same key with a direct, unbatched `put` (`OzoneManagerStateMachine.java:604`), using an index it computes from that not-yet-advanced value. A snapshot entering the gap between the commit and the advance therefore writes an index lower than what the commit just stored: 1. **OMDoubleBufferFlushThread** commits transactions 101..105. The DB now atomically holds their data and `TransactionInfo = 105`. It proceeds toward `accept(105)`. 2. **Ratis StateMachineUpdater** enters `takeSnapshotImpl` first. That method is `synchronized` and so is `updateLastAppliedTermIndex` — the same monitor — so step 1's `accept(105)` blocks. The window stays open across the put and the `flushDB` that follows rather than being a momentary read blip. 3. It reads `applied = 100`, computes `snapshot = 100`, and puts `TransactionInfo = 100`. That write is sequenced after the already-committed batch, so **100 overwrites 105**. The DB is then left holding the effects of 101..105 under a watermark that disclaims them, and because Ratis's snapshot index is also 100 the log purge stops there. Note the asymmetry the fix restores: `updateLastAppliedTermIndex` already guards the *in-memory* index against moving backwards (`assertUpdateIncreasingly`); the persisted twin had no equivalent. ### Approach The two writers are ordered on a lock owned by the double buffer, which owns the normal-path writer and the metadata manager. `takeSnapshotImpl` no longer writes directly; it goes through `persistIfNewer`, which does read-compare-write under that lock and returns whatever ends up stored. The snapshot uses that value for the DB row, the in-memory copy, and its return value, so the three cannot disagree. A read-then-write guard alone would not be enough: holding the state-machine monitor blocks the daemon's `accept()`, so no *new* batch can commit during a snapshot, but a commit already in flight can still land between the guard's read and its put. That leaves the same defect with a narrower window. The lock covers the batch commit and the read-compare-write, but not the snapshot's `flushDB`. Only the flush daemon commits and snapshots are rare, so it is effectively uncontended. One behavioural note for reviewers: `takeSnapshot()` can now return an index above `max(applied, notified)` when the buffer has committed further ahead. This looks safe — Ratis asserts the snapshot index does not exceed its own applied index, which counts submissions and is therefore at or above anything the buffer committed — but it is the one change beyond "never go backwards" and deserves a second opinion. ### On severity Deliberately not claimed. An earlier version of the Jira asserted this produces a startup crash-loop through the updateID guard in `WithObjectID.Builder.validate`. That did not survive checking — staging the regressed watermark on a real OM and restarting it produced a clean start with data intact — and the Jira is corrected accordingly. What is established is the state itself: the DB's own record of which transactions it contains is wrong. Establishing a concrete failure mode is open work, and severity should not be argued from one until then. Found while reviewing #10943, which touches the other writer of this field. ## What is the link to the Apache JIRA https://issues.apache.org/jira/browse/HDDS-16092 ## How was this patch tested? New unit tests, with each one's discriminating power checked by mutation rather than assumed: | Mutation | `testPersistIfNewerIsOrderedAgainstBatchCommit` | `testPersistedTransactionInfoNeverMovesBackwards` | `TestOzoneManagerStateMachine` | |---|---|---|---| | lock removed, comparison intact | **fails** | passes | passes | | comparison removed, lock intact | passes | **fails** | **fails** | | neither removed | passes | passes | passes | - `TestOzoneManagerDoubleBufferTransactionInfo` (new class) holds a snapshot between its read and its write while a real batch commit is attempted. It is the only test that fails when the lock is removed; the window is far too narrow to lose by chance. Its sibling runs commits and snapshots concurrently and asserts a reader never sees the index move backwards, which fails within a few rounds when the comparison is removed. - `TestOzoneManagerStateMachine` gains three cases covering the state machine's use of it: the stored index is not lowered when it is ahead, it is advanced when the snapshot's value is newer, and it is written when nothing is stored. The first fails on the unfixed code with `expected: <1#105> but was: <1#100>`. - The new tests live in their own class because the existing double-buffer tests assert on cumulative flush counters that any added commit inflates, and they keep the flush daemon stopped so the test thread is the only flusher, as in production — two concurrent flushers write indexes out of order for reasons unrelated to this change. Existing suites, all passing: `TestOzoneManagerStateMachine`, `TestOzoneManagerDoubleBuffer{,WithOMResponse,WithDummyResponse}` (71 tests), plus integration coverage for the risk this change actually introduces — a lock around every batch commit on the write path — via `TestOMRatisSnapshots` (7/7, real snapshots and checkpoint installs under load) and `TestOzoneManagerRestart` (3/3). The race itself is not reproduced by a test at any level, and deliberately so: a stress test hammering that path for 300 rounds never hit the window even with the lock removed, so an integration test carrying cluster overhead would be green with and without the fix. The staged interleaving is the honest substitute. -- 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]
