nizhikov commented on PR #13560: URL: https://github.com/apache/ignite/pull/13560#issuecomment-6142495967
Review round 3, head 11ec050310e. **Verdict: LGTM from my side, pending a green TC bot run.** Both blocking items from round 1 are resolved and verified, the intermediate test-compile breakage is gone, and the design was strengthened with lock coupling. Everything listed below is **non-blocking** (severity noted per item). ### Status of previous findings | # | Item | Status | |---|---|---| | 1 | TTL regression (`IgnitePdsWithTtlTest`) | Fixed by reverting the expired-old-row change. Verified: 2/2 full-class runs clean on the new head (3/4 failed before). | | 2 | `assertNotSame` on `long` | Fixed via static import of JUnit `assertNotEquals`. The intermediate `JUnitAssertAware.assertNotEquals(long, long)` helper, which shadowed JUnit's overloads for every test subclass (70 compile errors in 9 files), is gone. | | 3 | `storeCacheId()` flag set on only one init path | Fixed on all three entry points, plus `testInPlaceUpdateRowSizeCalculation` for shared group / eviction region. | | 4 | Data page scan torn reads | Confirmed: nothing in the codebase ever enables the thread-local; note left in IGNITE-11998. Also moot now that readers use lock coupling. | | 5 | 3/4 threshold | Commented. | | 6 | Coverage | Mostly closed: concurrent update, concurrent torn-read detection (get + ScanQuery), shared group, checkpoint recovery files, delta records after crash. WAL-disabled group remains untested (minor). | ### Design changes since round 2 (reviewed, look correct) - Lock coupling on both sides: `AbstractFreeList.updateFragmentedRow` holds the write lock on fragment N while acquiring N+1; `CacheDataRowAdapter.doInitFromLink` does the same with read locks. I traced lock orders against data-tree / pending-tree / index leaf locks and page eviction and found no cycle: only head pages are shared between rows, and a head page is always the first lock in any chain. - TTL restriction for multi-page rows dropped: safe, the pending tree reads the key from the head page only. - Compare-before-write also when WAL is off on persistent regions, so checkpoint recovery files don't pick up untouched pages. Covered by `testDirtyPagesCountAfterUpdateWithCheckpoinRecoveryFiles`. - `PageHandler.markDirtyAfterWrite` hook removed in favour of manual locking through `DataStructure` helpers. Cleaner. ### Remaining findings 1. **[Medium] Deadlock freedom rests on an assert-only invariant.** Lock coupling is safe only if non-head fragment pages are never shared. `writeWholePages` / `takePage` guarantee this for single and batch inserts, but the invariant is enforced only by an `assert` in `WriteRowHandler.addRow` at write time. With assertions disabled, data violating it (a future free-list change, a corrupted store) would deadlock instead of failing. A cheap runtime guard in `updateFragmentedRow` (e.g. next page `getDirectCount() == 1`, otherwise fall back to insert or throw a corruption error) would make that a safe path. 2. **[Low] Lock failure handled by assert only.** `lockFragmentPage` / `readLockFragmentPage` assert a non-zero address; with assertions off a zero address is dereferenced. The pre-existing reader had the same weakness, so this is hardening, not a regression. 3. **[Low] Page load under a held page lock.** `acquirePage` of the next fragment runs while the current fragment is write-locked, so on a persistent region a page miss does disk I/O under that lock, the entry lock and the checkpoint read lock. B+tree does the same, so acceptable, but worth a sentence in the javadoc. 4. **[Low] Feature flag evaluated twice.** `canUpdateOldRow` checks `isActive(...)` for large rows, then `RowStore.updateRow` evaluates it again for every in-place update, including small rows. Passing `allowFragmented` down would remove the duplicate. 5. **[Info] Rolling-upgrade bot warning is a false positive.** The new `coreFeatures` field in `IgniteNodeFeatureSet` carries no `@Order` and is not serialized. 6. **[Trivial] Typos** in new text: "milti-page" (`IgniteCacheOffheapManagerImpl`), "Checkpoin" (test name), "writting" (recovery test comment). 7. **[Info]** Merge base is 368f1f66f58, 14 commits behind master; a final rebase before merge is advisable. ### Verification on head 11ec050310e (local, fork profile, core built with upstream modules in a clean worktree) | Test class | Result | |---|---| | `MultiPageInPlaceUpdateTest` | 13/13 pass | | `IgnitePdsCheckpointRecoveryTest` | 8/8 pass | | `CacheFreeListSelfTest` | 20/20 pass | | `IgnitePdsWithTtlTest` (two runs) | 10/10 pass, both runs | Checkstyle, RAT and `check-test-suites` were not run locally; the new test is registered in `IgniteBasicTestSuite2`. -- 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]
