GigaMap: a failing reindex() no longer truncates the whole bitmap index group - #814
Conversation
…x, not to a detached one BitmapIndex#resolveFor returned this after merely validating the parent back-reference. That reference survives detachment: update(Indexer) replaces the registered instance, removeIndex drops it, and both release the dropped index' off-heap memory eagerly. A BitmapIndex obtained earlier from add() or get() therefore kept answering from a released, no longer maintained structure - silently as empty results rather than as a failure, because releasing nulls the segment slots instead of invalidating the object. Resolution is now by name and key type against the parent, so a held reference always reaches whatever is registered under it - which is what the identically named Indexer path (IndexIdentifier#resolveFor) has always done. A name that no longer resolves is reported instead of answered: the index was removed, or replaced by one with a different key type, and both are cases where returning nothing would be indistinguishable from a legitimately empty result. The foreign-parent guard is unchanged. This matters beyond the two existing entry points: making reindex() failure-atomic (internal#132) rebuilds each index into a replacement instance and swaps it in, which would otherwise turn every held reference stale as a side effect of a repair operation.
…ex group (internal#132) The rebuild dropped every index' data up front and then re-added entity by entity through internalAdd, which fans out to all indices of the group. An Indexer throwing at entity k therefore left every index - healthy siblings included - holding only the entities before k. The state before the failed recovery was stale but complete; afterwards it was silently partial, queries stopped finding entities they had found a moment earlier, and one ordinary mutation plus store() persisted the loss. That is the failure mode of the documented recovery path itself: reindex() is what the javadoc prescribes after a direct mutation or a class evolution of an indexed field, which is exactly when an indexer is most likely to throw. Each index is now rebuilt into a replacement built aside and swapped in only once its data is complete, the way update(Indexer) redefines one. A throwing indexer costs only its own index' rebuild: that index keeps its previous content - as stale as before the call, but complete - every other index is rebuilt, and none is ever a prefix of the entities. Every index is attempted before the first failure is rethrown, the rest suppressed, mirroring internalRemove. A unique-constraint violation is deliberately not treated as such a failure. There the rebuild did complete and merely produced colliding data, so the replacement is swapped in and the violation reported afterwards - the contract internal#121 established, which the repair depends on because it queries the rebuilt indices. Reporting before the swap would keep exactly the stale keys that made the rebuild necessary. The uniqueness check moves onto the replacement, where it is correct for the same reason it was correct after a clear: a replacement starts empty, so every hit is a genuinely different entity. Checking against the registered constraints during a rebuild would be wrong, since those still hold their full data and every entity trivially collides with itself. Rebuilding one index at a time costs one pass over the entities per index rather than one in total. That is deliberate: it bounds the extra memory to a single index' data - the peak update(Indexer) already has - instead of duplicating the whole group, which on a map sized for reindex() to matter is the difference between feasible and not. The commit sequence update(Indexer) used to inline is now a shared swapIndex, so the two paths cannot drift on unique-constraint and identity-index membership. reindex() gains the Behavior on failure section it was the only mutating method to lack, and IndexGroup.internalReindex documents that its default is neither validating nor failure-atomic. Lucene and vector groups still use a drop-first rebuild and are unchanged.
There was a problem hiding this comment.
Pull request overview
This PR hardens GigaMap’s bitmap index group so reindex() no longer leaves the entire index group silently truncated when a single Indexer throws, by rebuilding each index into a standalone replacement and swapping it in only once complete. It also fixes the stale held-BitmapIndex reference hazard introduced by swapping index instances by resolving handles via name/key-type at query time.
Changes:
- Make bitmap
internalReindex()rebuild indices one-by-one into replacements and atomically swap each completed index, collecting failures and suppressing subsequent ones. - Change
BitmapIndex#resolveForto re-resolve by(name, keyType)against the parent so held references can’t answer from released/off-heap data. - Add regression tests for the failure-atomic reindex behavior and held-index resolution; update docs and Javadoc to describe failure behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/issues/GigaMap132Test.java | Adds regression tests for failure-atomic reindex() behavior across sibling indices, durability, suppression, and identity preservation. |
| gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/indexer/HeldIndexReferenceTest.java | Adds tests ensuring held BitmapIndex handles resolve to the currently registered index and report removal/type changes. |
| gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/IndexGroup.java | Documents that the default group reindex behavior is not failure-atomic and may truncate on failures. |
| gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/GigaMap.java | Documents reindex() failure behavior and clarifies bitmap vs. other index-group semantics. |
| gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java | Implements per-index rebuild+swap for internalReindex(), moves uniqueness validation onto the replacement, and extracts shared swapIndex. |
| gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndex.java | Changes resolveFor to re-resolve stale handles via parent lookup and to report missing/retyped indices. |
| docs/modules/gigamap/pages/persistence.adoc | Documents reindex() failure semantics and guidance before persisting (store()). |
| docs/modules/gigamap/pages/crud.adoc | Documents the new failure-atomic behavior for bitmap indices during reindex(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… a garbled docs sentence The unique-enforcement assertion accepted any RuntimeException, which would have passed on an unrelated failure and masked exactly the regression it guards. It now names UniqueConstraintViolationExceptionBitmap. The persistence doc said the already rebuilt indices "are rebuilt"; it now says they keep their rebuilt data.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java:1047
- The comment says
internalRemoveIndex"strips both memberships", butinternalRemoveIndex(...)only removes the index frombitmapIndicesand fromuniqueConstraints; it does not touchidentityIndices(those are repointed later viainternalReplaceIdentityIndex). This wording is misleading for future maintenance/debugging.
// captured before the swap: #internalRemoveIndex strips both memberships
…rips The capture comment claimed internalRemoveIndex strips both the unique-constraint and the identity membership. It strips only the former, via internalRemoveUniqueConstraint; identity survives the removal and is re-pointed afterwards by internalReplaceIdentityIndex. Reading both before the swap is still right - they then describe the same pre-swap state - but the stated reason only held for one of them.
|
Round 2 had no new inline comments and one suppressed one.
(The inline comment that re-appeared on this head is the round 1 assertion one, already fixed in b1de66f and replied to there.) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/indexer/HeldIndexReferenceTest.java:157
- This assertion should use a boolean assertion rather than
assertEquals(true, ...)to avoid less-informative failure output and to match typical JUnit usage in this codebase.
assertEquals(true, e.getMessage().contains("no longer registered"));
…quals(true, ...) The message check reported only "expected true but was false" on failure. It now names what the message was supposed to say and prints the one it got, which is the difference between a one-line diagnosis and a debugging session.
|
Round 3 had one suppressed comment.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java:1223
addAsFailureis only used withRuntimeExceptionininternalReindex. Narrowing the helper’s parameter/return types toRuntimeExceptionkeeps the API honest and avoids forcing callers into widerThrowabletyping.
private static Throwable addAsFailure(final Throwable first, final Throwable next)
{
if(first == null)
{
return next;
}
first.addSuppressed(next);
return first;
gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java:1015
internalReindexonly catchesRuntimeException, but the accumulator is typed asThrowableand later cast back toRuntimeException. TypingfirstasRuntimeExceptionand throwing it directly removes the cast and prevents accidental widening to non-RuntimeExceptionvalues.
This issue also appears on line 1215 of the same file.
Throwable first = null;
try
{
for(final BitmapIndex.Internal<E, ?> existing : indices)
{
…imeException The best-effort loop catches RuntimeException but accumulated into a Throwable and cast back on the rethrow. The cast was the only thing keeping that honest: nothing but the catch clause prevented a non-RuntimeException from being collected, and the failure would then have been a ClassCastException at the very end of a rebuild. Narrowing the variable and addAsFailure to RuntimeException makes it a compile-time property and removes the cast.
|
Round 4 had two suppressed comments, both making the same point. The failure accumulator was typed The Full gigamap suite green (1216). |
|
Round 5 is clean - no new comments and no suppressed block - so I am closing the review loop here. Five rounds, four findings, all fixed:
Nothing touched the behavior under test: |
Fixes internal#132.
reindex()dropped every bitmap index' data up front and then re-added entity by entity throughinternalAdd, which fans out to all indices of the group. AnIndexerthrowing at entity k therefore left every index — healthy siblings included — holding only the entities before k. The state before the failed recovery was stale but complete; afterwards it was silently partial, queries stopped finding entities they had found a moment earlier, and one ordinary mutation plusstore()made the loss durable.That is the failure mode of the documented recovery path itself:
reindex()is what the javadoc prescribes after a direct mutation or a class evolution of an indexed field — exactly when an indexer is most likely to throw on a value it has never seen.Approach
Each index is rebuilt into a replacement built aside and swapped in only once its data is complete — the mechanism
update(Indexer)already uses. A throwing indexer costs only its own index' rebuild: that index keeps its previous content (as stale as before the call, but complete), every other index is rebuilt, and none is ever a prefix of the entities. Every index is attempted before the first failure is rethrown, the rest suppressed, mirroringinternalRemove.A unique-constraint violation is deliberately not such a failure. There the rebuild did complete and merely produced colliding data, so the replacement is swapped in and the violation reported afterwards — the contract internal#121 established, which the documented repair depends on because it queries the rebuilt indices. Reporting before the swap would keep exactly the stale keys that made the rebuild necessary.
GigaMap121Testis unchanged and green.The uniqueness check moves onto the replacement, where it is correct for the same reason it was correct after a clear: a replacement starts empty, so every hit is a genuinely different entity. Checking against the registered constraints during a rebuild would be wrong — those still hold their full data, so every entity trivially collides with itself.
Cost, chosen deliberately: rebuilding one index at a time means one pass over the entities per index rather than one in total. That bounds the extra memory to a single index' data — the peak
update(Indexer)already has — instead of duplicating the whole group. On a map sized forreindex()to matter, that is the difference between feasible and not.The commit sequence
update(Indexer)used to inline is now a sharedswapIndex, so the two paths cannot drift on unique-constraint and identity-index membership.Prerequisite commit: stale index references
Swapping index instances exposed an existing hole.
BitmapIndex#resolveForreturnedthisafter merely validating the parent back-reference — but that reference survives detachment, andupdate(Indexer)/removeIndexalready replace or drop the registered instance and release its off-heap eagerly. ABitmapIndexobtained earlier fromadd()orget()therefore kept answering from a released structure, silently as empty results rather than as a failure, because releasing nulls the segment slots instead of invalidating the object.Resolution is now by name and key type against the parent — which is what the identically named
Indexerpath (IndexIdentifier#resolveFor) has always done. A name that no longer resolves is reported instead of answered. Without this, makingreindex()atomic would have turned every held reference stale as a side effect of a repair operation.Documentation
reindex()gains the Behavior on failure section it was the only mutating method to lack.IndexGroup.internalReindex's javadoc now states that its default is neither validating nor failure-atomic.crud.adocandpersistence.adocdescribe the new guarantee;constraints.adocneeds no change — the completed-rebuild promise is preserved verbatim.Tests
GigaMap132Test(8 tests) — 5 are RED against the unfixed source, with the numbers from the issue: healthy sibling 4/5, durability probe 4/5, throwing index 3/5, and no suppressed second failure. The 3 that stay green are intentional pins: the remedy path, the #121 contract from inside the new code, and identity preservation.HeldIndexReferenceTest(5 tests) — 4 RED against the unfixedresolveFor. The fifth pins the preserved foreign-parent guard.Full suites green: gigamap 1216, lucene, jvector.
Not in this change
Lucene and vector groups still drop their data before rebuilding. Lucene's
internalRebuild(true)isdeleteAll+ back-fill on a singleIndexWriterand a twin cannot be opened on the same directory; the vector family has nointernalReindexoverride at all and owns an HNSW graph, a background executor and name-derived files. Both need their own design, to be filed alongside internal#143.