nizhikov commented on PR #13560: URL: https://github.com/apache/ignite/pull/13560#issuecomment-5661364383
Review of the change (head 936fcdbc6f9). The core mechanism (fragment-by-fragment in-place update with compare-before-write, feature-flag gated) looks sound and is well tested, but there is one reproducible regression and a broken test assertion that should be addressed before merge. ### Blocking **1. Regression in `IgnitePdsWithTtlTest.testConcurrentPutOpsToCacheWithExpirationCompleteSuccesfully`.** Local runs of the full test class: | Branch | Runs | Failures | |---|---|---| | PR head 936fcdbc6f9 | 4 | 3 | | merge-base d9e785f77fa | 3 | 0 | Every failure is a `CorruptedTreeException` in the pending tree during `purgeExpired`: a `PendingRow` points to a link whose item was already freed (`AssertionError: itemId=2, directCnt=2` in `CacheDataRowAdapter.doInitFromLink`, called from `PendingEntriesTree.getRow` / `BPlusTree$RemoveRange`). All failures were on the transactional caches (`expirable-cache-tx`, `expirable-cache-near-tx`) with 1 KB entries, so the multi-page code itself is not involved. The suspect is the change in `GridCacheMapEntry.UpdateClosure.call` (`GridCacheMapEntry.java:4379`): `this.oldRow` is now assigned before `checkRowExpired`, so an expired old row is handed to `finishUpdate`, which now removes its pending entry and frees its link. On master the tx path passed `null` plus `oldRowExpiredFlag`, so the expired row was left in place. I could not pin the exact interleaving by inspection, but the empirical signal is strong. Suggest splitting this change out (or reverting it here) and root-causing it under a separate ticket. Master's behavior looks like a row leak, so a fix is worthwhile, but it should not go in silently with this PR. This also needs a dedicated test. **2. Broken assertions in the new test.** `MultiPageInPlaceUpdateTest` uses `assertNotSame(link, link(ignite, key))` (lines 129 and 302). Both arguments are `long`, autoboxed into distinct `Long` objects, so the assertion always passes. Use `assertNotEquals`. The comment on line 300 claims links change after logical recovery; that is currently unverified. ### Should fix **3. `storeCacheId()` flag is set on only one init path.** `CacheDataRowAdapter` sets `FLAG_STORE_CACHE_ID` in `doInitFromLink` (line 303) but not in `initFromDataPage` or `initFromPageBuffer`. Rows produced by the data-page scan cursor (`CacheDataTree.java:261`) or the snapshot dump reader report `storeCacheId() == false` and `size()` four bytes short whenever the cache id is stored in pages. Nothing on the update path consumes those rows today, but the method contract is wrong. Set the flag wherever `readCacheId` is consumed (e.g. in `readIncomplete`). **4. Safety argument in `canUpdateOldRow` does not cover data-page scan.** The comment says entries are only read by link from the index tree, pending tree, and data tree under leaf lock. `ScanQuery.setDataPageScanEnabled` (and the JDBC equivalent) bypasses the data tree: `DataPageScanCursor` reads the head fragment under that page's lock and then follows `nextLink` page by page. A concurrent multi-page in-place update can produce a torn row for such readers. Before this PR fragmented rows were replaced, never rewritten. Either document the risk, disable data-page scan for such caches, or gate multi-page in-place update on it. **5. Unexplained threshold change.** `updateValSizeThreshold` moved from `pageSize / 2` to `pageSize * 3 / 4` with no comment. It now only gates the TTL and feature checks. Any row up to `pageSize - MIN_DATA_PAGE_OVERHEAD` is single-page anyway, so 3/4 is safe, but please explain it in the code or keep 1/2. **6. Missing coverage.** No tests for: shared cache group with `storeCacheIdInDataPage` (the format path `storeCacheId()` was introduced for); WAL-disabled group (`pageRecordsDisabled`); feature inactive during rolling upgrade; free-list level test in `CacheFreeListSelfTest` (only a stub method was added there); concurrent scan vs. in-place update. ### Minor / performance - `updateRowFragment` allocates a heap `ByteBuffer` per fragment page when WAL is enabled. A 100 KB row is ~25 allocations per update. A thread-local scratch buffer would remove that. - With WAL enabled, every fragment page is write-locked even if it ends up unmodified. Under a running checkpoint that triggers copy-on-write into the checkpoint buffer for pages that are never changed. Acceptable, but worth a comment. - `isActive(MULTI_PAGE_IN_PLACE_ROW_UPDATE_FEATURE)` is now evaluated in `RowStore.updateRow` for every in-place update, including small rows. It is cheap with the new `coreFeatures` fast path, but it could be evaluated only when `oldRow.size()` exceeds the single-page bound. - `oldRowExpiredFlag` is still declared and set in `AtomicCacheUpdateClosure` (line 4558) after its getter was removed from the interface; it is still used once (line 1526), so not dead, but the two closures now handle expiry differently. Consider unifying. - `DataPageUpdateResult` has a package-private constructor on a public class; fine, just noting. - `IgniteNodeFeatureSet.contains` fast path with `==` on the component-name constant is fine. ### What looks good - Fragment walk order matches `addRowFragment` (head first, `rowOff == written`); verified against `DataPageIO.writeFragmentData`. - Compare-before-write avoids dirtying and WAL-logging unchanged fragments; the `markDirtyAfterWrite` hook in `PageHandler` is a clean way to skip `setDirty`. - `updateRow(byte[])` for WAL replay now handles fragmented entries and asserts the stored length; the feature flag correctly protects downgraded nodes from silently skipping such records. - `storeCacheId()` replacing the `cacheId() != 0` heuristic removes the `oldLen -= 4` hack and the `DataRowCacheAware` subclass. - Crash/recovery tests (`testApplyInPlaceUpdateDeltaRecordsAfterCrash`, dirty page count test) exercise the snapshot + delta record path end to end. ### Verification done locally - Compiled ignite-core and upstream modules from the PR branch. - `MultiPageInPlaceUpdateTest`: 8/8 pass. `CacheFreeListSelfTest`: 20/20 pass. - `IgnitePdsWithTtlTest`: see the table above. - Checkstyle, RAT and `check-test-suites` were not run; 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]
