Repository navigation
TINKERPOP-3282 TinkerStorageGraph - #3639
spmallette wants to merge 94 commits into
Conversation
08f6937 to
8ecdb9a
Compare
e2bf480 to
931ce6d
Compare
|
I think one of the existing design choices made for the original TinkerTransactionGraph causes real problems with this implementation. The decision to make every attempt to lock that conflicted throw a TransactionException made sense when TinkerTransactionGraph was just for unit testing transactions. However, since this is now supposed to be a fully capable production-ready Graph, that logic doesn't really hold. When there are multiple users, even moderately concurrent workloads might result in immediate TransactionExceptions that the user will need to retry themselves. Some choices made here exacerbate the situation.
So lock contention was already going to be a problem before, but this current design requires you hold these locks for even longer. I don't think users will find it acceptable that they are constantly getting TransactionExceptions. We probably need to move to a lock wait solution which while increases the chances of deadlock or we drastically reduce the time you need to hold the locks and add retry for transactions into the GLVs. Either would probably result in a better user experience. |
| final long fileLength = file.length(); | ||
| try (final DataInputStream in = new DataInputStream(new BufferedInputStream(new FileInputStream(file)))) { | ||
| long remaining = readAndVerifyHeader(in, file, fileLength); | ||
| while (true) { |
There was a problem hiding this comment.
Replay correctly stops at a torn trailing frame, but the log is never cut back to the end of the last complete frame. New commits are then appended after the torn bytes. If the process crashes again before the next compaction or clean close(), replay stops at that old torn frame and drops every commit written since, or reads into them, fails the CRC check, and refuses to open. The log should be cut back to the last good frame during replay, before anything is appended.
There was a problem hiding this comment.
Fixed in 4c7caa1: replay now cuts the log back to the end of its last complete frame before anything is appended, and logs a warning with how many bytes it dropped. A log torn inside its own header is now treated as empty instead of refusing to open. New tests cover both outcomes you described, silent loss and the CRC refusal, by crashing a second time after commits made on top of a torn tail.
| changedVertices.forEach(v -> v.commit(txVersion)); | ||
| changedEdges.forEach(e -> e.commit(txVersion)); | ||
| // write-ahead: durably persist the changeset before applying the in-memory commit, so a failure here | ||
| // aborts the commit (via the catch below) and leaves memory and disk consistent. Skipped while the graph |
There was a problem hiding this comment.
Is this actually true? persist and flush throw UncheckedIOException which isn't covered by TransactionException.
There was a problem hiding this comment.
A failed persist/flush skipped the rollback, so the failed changes stayed visible and in the indices, and the next flush could still write the transaction to disk. Fixed in dd8a234: doCommit now rolls back on any failure, and after a log write or flush failure the engine refuses further commits until the graph is reopened. A failed compaction no longer fails the commit it follows. Writing the tests also turned up an older NullPointerException in doRollback, fixed separately in a18db6f.
| public void persist(final long txVersion, | ||
| final Collection<TinkerStorageMutation<TinkerVertex>> changedVertices, | ||
| final Collection<TinkerStorageMutation<TinkerEdge>> changedEdges) { | ||
| ensureLogOpen(); |
There was a problem hiding this comment.
A commit can race with close() since ensureLogOpen doesn't actually check if its closed and the flush does check and returns early.
There was a problem hiding this comment.
Fixed in 7b183f1: persist and ensureLogOpen now throw once the engine is closed, and the commit is rolled back. closed is set under the same commit lock persist runs under, so a commit that was waiting on close() now fails cleanly. Before, it reopened the log, never flushed, and reported success. A new conformance test checks that a commit after close() fails and leaves the storage directory unchanged.
|
|
||
| // Attach vertices first so edges can find their endpoints, then commit once. | ||
| for (final DetachedVertex v : vertices.values()) | ||
| v.attach(Attachable.Method.getOrCreate(graph)); |
There was a problem hiding this comment.
I'm not sure we want to support property cardinality but there maybe a problem here if we do. AI summary below:
With the default
defaultVertexPropertyCardinality=single, a user can still writeproperty(list, 'k', 'a')andproperty(list, 'k', 'b')and commit, and both values are logged (writeVertexRecordgroups them by key). On reopen,getOrCreateapplies the graph-widesinglecardinality and only the last value survives. Silently losing data from acknowledged commits breaks the durability contract. Documenting it, as line 481 does, isn't enough. This differs fromio()import because the graph is reloading its own data. The record already stores how many values each key has: replay should uselistwhen there is more than one, or the format should record cardinality per key. Please add a test with defaultsingle: write a list property, commit, reopen, and check both values are present.
There was a problem hiding this comment.
yes, that's a problem. i opted to:
- not store cardinality per property value
- do what TinkerGraph has always forced and use the default setting for TinkerGraph to guide incoming data - if you have any multi-properties then you better make it the default
- wait for schema and hopefully fix this properly
There was a problem hiding this comment.
AI review flagged a problem about corrupt stores being unrepairable. As follows:
One bad CRC, unknown opcode or undefined dictionary reference anywhere in the snapshot or log makes the whole store unopenable. The only guidance ("export via g.io()") needs a graph that opens, and the same is true after a format-version mismatch.
So we need an actual recovery mode since g.io() doesn't work.
There was a problem hiding this comment.
Added gremlin.tinkergraph.storage.recover in cf98a77. It opens a damaged store read-only with everything still readable: log replay stops at the first bad transaction, unreadable snapshot elements are skipped along with edges that lose an endpoint, and a warning lists what was left out. The files are left untouched so the graph can be exported with g.io(). For a format-version mismatch, the error now says to open the store with the TinkerPop version that wrote it and export from there. Can probably improve on this in the future.
| * file. They are left in place here only to keep existing credential configurations working. A graph with a | ||
| * storage engine manages its own data and is skipped. | ||
| */ | ||
| private static void loadCredentialStore(final TinkerGraph graph) { |
There was a problem hiding this comment.
Out of scope for this PR, but I feel like SimpleAuthenticator should be completely reworked for TinkerPop 4. It was already feeling dated, but stitching it to removed Graph config options really emphasizes that further.
There was a problem hiding this comment.
yeah.........i noticed that and figured we'd be coming for that issue someday, so i just left the settings as they were.
| # Sample configuration for a durable, transactional TinkerStorageGraph on Gremlin Server. Reference it from the | ||
| # server's "graphs" block, for example in gremlin-server-transaction.yaml: |
There was a problem hiding this comment.
I think the extra reference creates a maintenance chore without adding much value. Also it's referencing how gremlin-server-transaction.yaml references this file, yet the server config yamls are unchanged in this PR.
| # Sample configuration for a durable, transactional TinkerStorageGraph on Gremlin Server. Reference it from the | |
| # server's "graphs" block, for example in gremlin-server-transaction.yaml: | |
| # Sample configuration for a durable, transactional TinkerStorageGraph on Gremlin Server. |
| @Override | ||
| public void close() { | ||
| if (graphLocation != null) saveGraph(); | ||
| if (storage != null) { |
There was a problem hiding this comment.
Nit: I wonder if we're better off pushing this down to the concrete classes. It feels a bit weird having logic concerning only TinkerStorageGraph in the shared abstract class.
There was a problem hiding this comment.
created an issue for this: https://issues.apache.org/jira/browse/TINKERPOP-3291
I had to make a decision around this early on and decided that for this first effort it would be acceptable to take these limitations around locking to at least cover the current use of TinkerGraph as typically in-memory with a single writer - there are still many production use cases for that type of operation. I wanted this round of work to at least satisfy that case without also making this a transaction refactoring on top of that. As you call this out, I sense that not enough has been written in the docs to be clear about the limitation and where TinkerStorageGraph is best used. |
cf98a77 to
1a1add0
Compare
937bee5 to
3fa7b54
Compare
TinkerStorageGraph can now durably persist committed transactions to disk through a pluggable storage engine, selected with the new gremlin.tinkergraph.storage config key with graphLocation as the storage directory. A GraphBinary write-ahead-log engine ships as the reference implementation; custom engines implement the TinkerStorage SPI. TinkerMemoryGraph is now purely in-memory: the old load-on-open / save-on-close behavior and the graphFormat key are removed. Use TinkerStorageGraph for durability or g.io() for interchange. SimpleAuthenticator now reads its credential store explicitly since the in-memory graph no longer auto-loads. Assisted-by: Claude Code:claude-opus-4-8
The pluggable storage and TinkerMemoryGraph persistence-removal entries were landing in the released beta.3 section; move them up beside the related TinkerGraph refactoring entries in the unreleased 4.0.0 section. Assisted-by: Claude Code:claude-opus-4-8
…c mode The GraphBinary storage engine's flush() only pushed bytes into the OS page cache, so an acknowledged commit could be lost on OS crash or power loss. flush() now fsyncs on commit, and compaction is made crash-safe: the snapshot is fsync'd and atomically renamed into place with directory fsyncs before the log is truncated. A new gremlin.tinkergraph.storage.sync config key selects the durability mode: 'commit' (default, fsync every commit) or 'os' (flush to the OS only; survives process crash but not power loss). Assisted-by: Claude Code:claude-opus-4-8
TinkerGraph transactions lock only their own changed elements, so commits touching disjoint elements ran their commit paths concurrently and both wrote to the storage engine's single append log at once, interleaving and corrupting its records. A fair per-graph commit-write lock now serializes the engine's persist/flush (and the compact/close paths that rewrite the same files), held only around the durable write so disjoint commits still proceed in parallel up to that point. Assisted-by: Claude Code:claude-opus-4-8
A persistent TinkerStorageGraph is a single-writer embedded store, but nothing stopped a second graph — in the same JVM or another process — from opening the same location and corrupting its log and snapshot. open() now takes an exclusive OS advisory lock on a LOCK file in the storage directory, held for the graph's lifetime and released on close(); a second open fails fast with a clear error. An OS lock is used so the kernel releases it if the JVM dies. Assisted-by: Claude Code:claude-opus-4-8
A long-running graph that is never explicitly closed grew its append log without bound, and its restart replay cost grew with it. The GraphBinary engine now tracks appended log bytes and folds the log into a snapshot on commit once it exceeds a threshold, configurable via gremlin.tinkergraph.storage.compactThreshold (default 64MB, 0 to disable). A new no-op-by-default TinkerStorage.maybeCompact SPI hook drives this, so existing engines are unaffected. Compaction runs inline under the commit lock. Assisted-by: Claude Code:claude-opus-4-8
Compaction built the entire graph snapshot as a single in-heap byte array (with a doubling buffer), so compacting a large graph needed a second full copy of it in memory and risked OOM. The snapshot is now streamed one element per framed record straight to the file, bounding peak memory to a single element. The on-disk format is unchanged: each frame is an ordinary single-entry put record that replay folds exactly as before. Write amplification (a commit rewrites each changed element in full) is documented as a known limitation rather than mitigated, since elements are small and auto-compaction bounds log growth. Assisted-by: Claude Code:claude-opus-4-8
Storage files could silently misread corrupt data: a bit-flip inside a frame went undetected, and a garbage frame length was indistinguishable from a truncated trailing append. Each frame now carries a CRC32, and every file starts with a magic + version header. On read, a complete frame with a bad CRC or a file with bad magic / an unknown version fails loudly, while a short trailing frame is still tolerated as an interrupted append. The payload allocation is bounded by the remaining file bytes, so a corrupt length can no longer trigger a huge allocation. The per-record version byte is dropped in favor of the file header. This changes the on-disk format, which is safe as the storage feature is unreleased. Assisted-by: Claude Code:claude-opus-4-8
…h storage Fills the storage test gaps left after the durability/integrity work. StorageCrashConsistencyTest reconstructs the exact on-disk states a crash leaves at each step of the write-ahead commit and compaction sequences — durable-commit-without-snapshot, snapshot-plus-log, stray temp snapshot before rename, and new-snapshot-with-log-not-yet-deleted — and asserts each reopens to the correct graph, deterministically and without killing a JVM. GraphBinaryStorageTest gains a scaled snapshot-streaming test (500 vertices + 499 edges must write exactly one frame per element) as a bounded-memory proxy, and documents why real fsync/power-loss durability is out of unit-test scope (needs OS-level fault injection). Assisted-by: Claude Code:claude-opus-4-8
The format version lived in every file header, so a store holding a snapshot and log written at different times could carry two versions that disagree. It now lives once in a store-level VERSION marker (magic + version, fsynced), the single source of truth; per-file headers keep only the magic for identity and corruption detection. Opening a store whose marker names an unsupported version fails loudly and points to g.io() export as the migration path — the engine deliberately does no in-place migration. Unknown record opcodes remain a hard error, so a genuinely incompatible change bumps the version. Assisted-by: Claude Code:claude-opus-4-8
Bring the TinkerGraph reference persistence section up to date with the storage engine's settings: the gremlin.tinkergraph.storage.sync durability modes (commit/os) and gremlin.tinkergraph.storage.compactThreshold auto-compaction knob are added to the configuration table, and the persistence prose now covers commit durability, log compaction, single-writer directory locking, and the storage-format version check with g.io() export as the migration path. Assisted-by: Claude Code:claude-opus-4-8
Move the codec-agnostic log-structured machinery — file layout, VERSION marker, CRC framing, replay fold, SyncMode durability, and crash-safe + threshold compaction — into a new abstract AbstractLogStorage. GraphBinaryStorage now supplies only the element codec through four hooks (encodeCommit, decodeFrame, writeSnapshot, beginReplay). Pure refactor: on-disk format and behavior are unchanged, so a new codec can be built by overriding the hooks alone. Assisted-by: Claude Code:claude-opus-4-8
Replace whole-DetachedVertex/DetachedEdge GraphBinary serialization with a component codec: element ids and property values go through GraphBinary's scalar serializers (a one-byte DataType tag + raw value), while labels, property keys, and meta-property keys are dictionary-encoded to small integer refs. New keys are emitted as OP_DICT_APPEND records within the same frame, before their first use. Vertex-property ids are auto-generated and no longer persisted (regenerated on load); element and edge ids are preserved. Compaction preserves dictionary numbering and writes the whole dictionary as a self-contained snapshot header, and dict-append decode is idempotent, so a log surviving the compaction crash window still resolves its refs. This removes the old whole-object codec (no dead code) and drops per-element key/label repetition and the per-property envelope, shrinking the on-disk footprint. Assisted-by: Claude Code:claude-opus-4-8
Add gremlin.tinkergraph.storage.preserveVertexPropertyIds (default false). When enabled, the GraphBinary codec persists each vertex-property id so it is stable across reopen; by default those ids are regenerated on load to keep the store smaller. A per-vertex-record flag makes each record self-describing, so a store written with the option reopens correctly even if the reader's setting differs. A new configureCodec hook lets the codec read its own config at open. Also adds codec tests: dictionary growth across log commits, dictionary rewrite on compaction, preserve-ids on/off, and a bytes/element size-regression guard (well under the ~168 bytes/element the old whole-object format cost). Assisted-by: Claude Code:claude-opus-4-8
…ption Add gremlin.tinkergraph.storage.preserveVertexPropertyIds to the TinkerGraph configuration table and note in the persistence section that element and edge ids are always preserved on reopen while auto-generated vertex-property ids are regenerated by default. Fold the compact dictionary encoding and the new option into the storage CHANGELOG entry. Assisted-by: Claude Code:claude-opus-4-8
…rips Add conformance round-trip coverage for the storage codec's breadth: a value-type matrix (int/long/float/double/boolean/byte/short/char/string/UUID/ BigInteger/BigDecimal/OffsetDateTime/Duration), collection-valued properties (List/Map/Set), null values, heterogeneous same-key types, non-Long (UUID and String) element ids, unicode keys and values, and a large-schema graph whose >127 distinct keys and >127 values under one key exercise the multi-byte varint path that small graphs never reach. Assisted-by: Claude Code:claude-opus-4-8
Add TCK cases for repeated compaction cycles, multiple open/close sessions, and concurrent commits with distinct keys. Add a crash-consistency test for the compaction crash window where a dead dictionary key forces the surviving log to diverge from the new snapshot's dictionary — guarding the decision to preserve dictionary numbering across compaction rather than renumber. Assisted-by: Claude Code:claude-opus-4-8
Surface an unknown value type code as a Corrupt storage IOException instead of an opaque NullPointerException, matching the codec's other integrity checks. Add decode-path tests for the four malformed-frame cases that clear CRC framing but are internally invalid: unknown op code, dictionary append gap, dictionary id redefinition, and unknown value type code. Assisted-by: Claude Code:claude-opus-4-8
…ty conflict Add a comment explaining why supportsConcurrentAccess() is false: a persistent TinkerStorageGraph is a single-writer store (DirectoryLock), and the feature denotes multiple connections/instances sharing the same data, not the intra-instance multi-thread transaction access the graph already provides. Re-enable the stale commented-out fail() in the concurrent meta-property test and strengthen its assertion so it proves the losing transaction rolled back, turning a decorative test into a real guard for conflict detection. Assisted-by: Claude Code:claude-opus-4-8
Assisted-by: Claude Code:claude-opus-5-5
Assisted-by: Claude Code:claude-opus-5-5
…aced Assisted-by: Claude Code:claude-opus-5-5
…ail the open Assisted-by: Claude Code:claude-opus-5-5
…very open Assisted-by: Claude Code:claude-opus-5-5
… as durable Assisted-by: Claude Code:claude-opus-5-5
…he last one Assisted-by: Claude Code:claude-opus-5-5
…while the graph was cleared Assisted-by: Claude Code:claude-opus-5-5
… as damage Assisted-by: Claude Code:claude-opus-5-5
…g labels read by type Assisted-by: Claude Code:claude-opus-5-5
… compaction Assisted-by: Claude Code:claude-opus-5-5
…d charset Assisted-by: Claude Code:claude-opus-5-5
Assisted-by: Claude Code:claude-opus-5-5
…nt read-only opens to recovery Assisted-by: Claude Code:claude-opus-5-5
Assisted-by: Claude Code:claude-opus-5-5
Assisted-by: Claude Code:claude-opus-5-5
…ged since the last compaction Assisted-by: Claude Code:claude-opus-5-5
… transaction on its id ends Assisted-by: Claude Code:claude-opus-5-5
…n unwritten sectors Assisted-by: Claude Code:claude-opus-5-5
…ic compaction throws an Error Assisted-by: Claude Code:claude-opus-5-5
…nores Assisted-by: Claude Code:claude-opus-5-5
…nstall its empty log Assisted-by: Claude Code:claude-opus-5-5
Assisted-by: Claude Code:claude-opus-5-5
…erStorageGraph in a test Assisted-by: Claude Code:claude-opus-5-5
Assisted-by: Claude Code:claude-opus-5-5
… read past a damaged one Assisted-by: Claude Code:claude-opus-5-5
… damaged settings file Assisted-by: Claude Code:claude-opus-5-5
…by URL Assisted-by: Claude Code:claude-opus-5-5
Assisted-by: Claude Code:claude-opus-5-5
… the same JVM is refused Assisted-by: Claude Code:claude-opus-5-5
3 participants
https://issues.apache.org/jira/browse/TINKERPOP-3282
Adds a pluggable storage option to TinkerGraph allowing it to persist data to disk as part of its transaction model. For simplicity the default storage implementation uses GraphBinary as the storage format. Review the Upgrade Documentation changes for details.