Couple external-directory Lucene index commits to GigaMap.store() (#715)#719
Merged
Conversation
The LuceneIndex committed eagerly on every mutation. With the default embedded GraphDirectory that is harmless (the commit only touches the in-memory fileEntries map, persisted on store()), but with an external MMap(path) directory each mutation was flushed to disk immediately, decoupled from store(): a mutation not followed by a store (or a crash in between) left the on-disk index diverged from the persisted entities. Make the commit behavior selectable and store-aligned: - LuceneContext: add a public New(directoryCreator, analyzerCreator, documentPopulator, autoCommit) factory. Store the flag inverted as manualCommit so the false-default of an added field preserves the legacy autoCommit==true behavior when loading pre-existing stores. - LuceneIndex.Default.internalCommitOnStore(): flush+commit the writer when autoCommit is disabled and there are uncommitted changes. - BinaryHandlerLuceneIndexDefault: invoke internalCommitOnStore() before serializing, so the writer is flushed into fileEntries (graph) / to disk (external) exactly at the store() boundary. This also fixes a latent bug where a graph index with autoCommit=false persisted an empty/stale fileEntries map. Document the autoCommit x directory-type matrix and the remaining external-directory caveat (index and GigaMap storage are separate targets, so coupling aligns the commits temporally but is not a single atomic transaction). Add LuceneStoreBoundaryCommitTest covering MMap store-boundary commit (and no eager commit) plus GraphDirectory reload with autoCommit=false.
Contributor
There was a problem hiding this comment.
Pull request overview
Couples Lucene index commit behavior to the GigaMap.store() boundary (when autoCommit=false) so that external-directory indexes don’t durably diverge from persisted entities due to eager mutation-time commits, while preserving legacy behavior (autoCommit=true) for existing users.
Changes:
- Adds a public
LuceneContext.New(..., autoCommit)factory overload and persists the setting via an invertedmanualCommitflag to maintain backward-compatible defaults on reload. - Introduces a store-boundary commit hook (
internalCommitOnStore) and invokes it fromBinaryHandlerLuceneIndexDefault.store(...)before serialization. - Adds a new test covering store-boundary commit semantics for both MMap (external) and GraphDirectory (embedded) modes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| gigamap/lucene/src/main/java/org/eclipse/store/gigamap/lucene/LuceneContext.java | Exposes autoCommit configuration via a new factory overload and documents directory-type semantics. |
| gigamap/lucene/src/main/java/org/eclipse/store/gigamap/lucene/LuceneIndex.java | Adds store-boundary commit behavior when autoCommit=false and updates commit API docs accordingly. |
| gigamap/lucene/src/main/java/org/eclipse/store/gigamap/lucene/BinaryHandlerLuceneIndexDefault.java | Hooks the store-boundary commit into persistence by committing before LuceneIndex serialization. |
| gigamap/lucene/src/test/java/org/eclipse/store/gigamap/lucene/LuceneStoreBoundaryCommitTest.java | Verifies external and embedded directory behavior matches store-boundary commit expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zdenek-jonas
approved these changes
Jun 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Fixes #715.
The
LuceneIndexforGigaMapcommitted eagerly at mutation time — everyinternalAdd/internalAddAll/internalRemove/internalRemoveAll/internalUpdateIndicesflushed and committed theIndexWriterimmediately (becauseLuceneContext.autoCommit()defaulted totrue). The actual consistency guarantee then silently depended on the directory type:autoCommit=true(default)autoCommit=falsefileEntriesmap)store(). Consistent at store boundaries.fileEntries, sostore()could persist an empty/stale index.store(). A mutation with no followingstore()(or a crash in between) left the on-disk index diverged from the persisted entities.commit()was called manually.This contradicts the "save it when I tell you to save" expectation (discussion #708).
On top of that,
autoCommit=falsewas effectively unreachable for external users:LuceneContext.Default's constructor is package-private and no factory exposed the flag.Changes
LuceneContext— added a public factory overloadNew(directoryCreator, analyzerCreator, documentPopulator, autoCommit).The existing four factories still default to
autoCommit=true, so behavior is unchanged for current users. The flag is stored inverted asmanualCommit:a field added to a persisted type defaults to
falsewhen an older store is loaded, and storingmanualCommitmakes that default meanautoCommit()==true, preserving the legacy behavior on reload with no migration.LuceneIndex.Default.internalCommitOnStore()— flushes + commits the writer whenautoCommitis disabled and there are uncommitted changes (no-op otherwise).BinaryHandlerLuceneIndexDefault— overridesstore(...)to callinternalCommitOnStore()before serialization. The ordering matters:super.storestores the
fileEntriesreference and then its contents, so the commit must populatefileEntriesfirst. This couples the commit to theGigaMap.store()boundary for both directory types and fixes the latent GraphDirectory stale-persist bug.autoCommit× directory-type matrix onautoCommit()and the remaining external-directory caveat: the index and the GigaMap storage are separate persistence targets, so coupling aligns the commits temporally but is not a single atomic transaction across both (a crash mid-store()can still diverge). The embedded graph directory has no such gap.Behavior
autoCommit=true) is unchanged.autoCommit=false, the writer is flushed+committed exactly once, at theGigaMap.store()boundary (or via an explicitcommit()between stores).Testing
gigamap-lucenesuite green (61 tests).LuceneStoreBoundaryCommitTest:store()calls, not mutations — proving store-boundary commit and no eager commit.autoCommit=false.