Four independent findings from the work on #6217 (merged as 2fcf408ec, PR #6224). Filed together because they were found together; each is separable and can be taken on its own. Items 1 and 2 are in LocalBucket.loadMultiPageRecord, items 3 and 4 are test/CI issues found on that PR's run.
Line numbers below are against 2fcf408ec.
1. Under REPEATABLE_READ, a conflicting chunked read burns its whole retry budget on retries that cannot possibly succeed
The retry branch re-fetches the head page straight from the PageManager, deliberately bypassing the transaction so that the retry sees fresh data (LocalBucket.java:3322):
firstPage = database.getPageManager().getImmutablePage(firstPageId, pageSize, false, true);
The chain walk, however, takes its continuation pages from the transaction (LocalBucket.java:3269):
final BasePage nextPage = database.getTransaction()
.getPage(new PageId(database, file.getFileId(), chunkPageId), pageSize);Under READ_COMMITTED those two agree - the transaction caches nothing, both are the newest committed page - so a retry can genuinely succeed, which is what it is there for.
Under REPEATABLE_READ they cannot agree. TransactionContext.getPage caches every page walked in immutablePages, so each retry pairs a fresh head chunk with stale cached tails - precisely the torn mix validateChainRead then rejects. The cache is never refreshed, so every subsequent attempt reproduces the same mismatch. The read therefore always performs TX_RETRIES + 1 complete chain walks (4 by default) before raising the ConcurrentModificationException it was always going to raise.
Two things to decide:
- It should fail fast. Retrying cannot help at this isolation level, so the budget is pure waste on a path that is already the slow one - and it is worst exactly where it hurts, on large records under contention.
- Arguably it should not fail at all. The transaction's cached chain is internally consistent: it is one committed state, the one this transaction first saw, which is what
REPEATABLE_READ promises. Serving that snapshot would be more correct than refusing it. That is a semantic change rather than a patch, so it wants a decision first.
Issue6217ChunkedReadFalseConflictTest.aChunkedReadStillFailsWhenTheRecordItselfMovedUnderIt pins the current behaviour and is the place to observe it.
2. A structurally broken chain is retried as if it were contention, and reported as contention
loadMultiPageRecord collapses two conditions with opposite answers into one chainInconsistent flag:
- the chain does not parse - a chunk pointer past the end of the file (
:3264), a slot that no longer holds a record (:3273), a marker that is not NEXT_CHUNK (:3289), or any exception from the walk (:3295). This is a corrupt record: retrying cannot change the answer.
- the record changed under the read (
:3307, from validateChainRead). This is contention: retrying is the entire point.
Both take the same retry path and end at the same message:
Multi-page record #1:26 was modified during read after 3 retries. Please retry the operation
So a corrupt record costs 4 full chain walks on every read of it, and then reports a concurrency problem to whoever reads the log - who goes looking for contention that does not exist. BucketIteratorBrokenChunkChainTest shows the misleading message today:
SEVER [BucketIterator] Error on loading record #1:0 (error: Multi-page record #1:0 was modified during read after 3 retries. Please retry the operation)
The fix is cheap because the walk already knows which of the two it hit - the structural branches are distinct from the validateChainRead verdict. Failing fast on a broken chain, with a message that says the chain is broken, would also make chunkChainReadRetries (added in #6217) mean one thing instead of two.
3. Flaky: TimeSeriesTagDictionaryTest.aSecondWaveOfIdsIsAlsoResolvedByReloading
Failed on the unit-tests lane of PR #6224 (job), passed on a rerun of the same commit:
TimeSeriesTagDictionaryTest.aSecondWaveOfIdsIsAlsoResolvedByReloading:198
expected: "host_b"
but was: null
Line 198 is the first wave (follower.getById(2)), i.e. the reload that the single-wave test already covers, not the second-wave path the test was written for.
Worth treating as possibly a real race in the dictionary's self-healing reload rather than only test timing: a null there is exactly the failure the self-heal exists to prevent, and an HA follower hitting it would read null for a tag that is on disk. It did not reproduce locally - 9 consecutive runs of the class green, plus green inside a full 11993-test engine lane - so it needs the CI environment (or an equivalent load) to reproduce.
4. Flaky: LSMVectorIndexAutoCompactionTest.anUpdateHeavyIndexCompactsItselfWithoutAnExplicitCommand
Failed on the slow-unit-tests lane of the same run (job), passed on rerun:
LSMVectorIndexAutoCompactionTest.anUpdateHeavyIndexCompactsItselfWithoutAnExplicitCommand:81
[the workload must first grow the file well past the live set, or nothing is tested]
The assertion that fails is the test's own precondition, not its subject: the workload did not grow the file as far as the test needs before it starts measuring. A test that cannot establish its precondition under load is a test that silently stops testing whenever the machine is busy, so the workload should be made deterministic rather than the threshold relaxed.
Four independent findings from the work on #6217 (merged as
2fcf408ec, PR #6224). Filed together because they were found together; each is separable and can be taken on its own. Items 1 and 2 are inLocalBucket.loadMultiPageRecord, items 3 and 4 are test/CI issues found on that PR's run.Line numbers below are against
2fcf408ec.1. Under
REPEATABLE_READ, a conflicting chunked read burns its whole retry budget on retries that cannot possibly succeedThe retry branch re-fetches the head page straight from the
PageManager, deliberately bypassing the transaction so that the retry sees fresh data (LocalBucket.java:3322):The chain walk, however, takes its continuation pages from the transaction (
LocalBucket.java:3269):Under
READ_COMMITTEDthose two agree - the transaction caches nothing, both are the newest committed page - so a retry can genuinely succeed, which is what it is there for.Under
REPEATABLE_READthey cannot agree.TransactionContext.getPagecaches every page walked inimmutablePages, so each retry pairs a fresh head chunk with stale cached tails - precisely the torn mixvalidateChainReadthen rejects. The cache is never refreshed, so every subsequent attempt reproduces the same mismatch. The read therefore always performsTX_RETRIES + 1complete chain walks (4 by default) before raising theConcurrentModificationExceptionit was always going to raise.Two things to decide:
REPEATABLE_READpromises. Serving that snapshot would be more correct than refusing it. That is a semantic change rather than a patch, so it wants a decision first.Issue6217ChunkedReadFalseConflictTest.aChunkedReadStillFailsWhenTheRecordItselfMovedUnderItpins the current behaviour and is the place to observe it.2. A structurally broken chain is retried as if it were contention, and reported as contention
loadMultiPageRecordcollapses two conditions with opposite answers into onechainInconsistentflag::3264), a slot that no longer holds a record (:3273), a marker that is notNEXT_CHUNK(:3289), or any exception from the walk (:3295). This is a corrupt record: retrying cannot change the answer.:3307, fromvalidateChainRead). This is contention: retrying is the entire point.Both take the same retry path and end at the same message:
So a corrupt record costs 4 full chain walks on every read of it, and then reports a concurrency problem to whoever reads the log - who goes looking for contention that does not exist.
BucketIteratorBrokenChunkChainTestshows the misleading message today:The fix is cheap because the walk already knows which of the two it hit - the structural branches are distinct from the
validateChainReadverdict. Failing fast on a broken chain, with a message that says the chain is broken, would also makechunkChainReadRetries(added in #6217) mean one thing instead of two.3. Flaky:
TimeSeriesTagDictionaryTest.aSecondWaveOfIdsIsAlsoResolvedByReloadingFailed on the
unit-testslane of PR #6224 (job), passed on a rerun of the same commit:Line 198 is the first wave (
follower.getById(2)), i.e. the reload that the single-wave test already covers, not the second-wave path the test was written for.Worth treating as possibly a real race in the dictionary's self-healing reload rather than only test timing: a
nullthere is exactly the failure the self-heal exists to prevent, and an HA follower hitting it would readnullfor a tag that is on disk. It did not reproduce locally - 9 consecutive runs of the class green, plus green inside a full 11993-test engine lane - so it needs the CI environment (or an equivalent load) to reproduce.4. Flaky:
LSMVectorIndexAutoCompactionTest.anUpdateHeavyIndexCompactsItselfWithoutAnExplicitCommandFailed on the
slow-unit-testslane of the same run (job), passed on rerun:The assertion that fails is the test's own precondition, not its subject: the workload did not grow the file as far as the test needs before it starts measuring. A test that cannot establish its precondition under load is a test that silently stops testing whenever the machine is busy, so the workload should be made deterministic rather than the threshold relaxed.