kv: use one-phase commit for single-group txns#382
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant optimization for single-group transactions by implementing a one-phase commit mechanism. This change streamlines the transaction lifecycle for these specific cases, improving efficiency by reducing the number of steps required for a successful commit. The core idea is to bypass the traditional two-phase commit overhead when a transaction's operations are confined to a single transactional group, leading to faster and more resource-efficient operations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant optimization by implementing a one-phase commit protocol for single-group transactions, which avoids the overhead of the two-phase commit process. The changes are logical and well-structured across the coordinator, FSM, and transaction manager. The new Phase_NONE transaction type is handled correctly, and the decision to skip abort cleanup for these transactions is sound. The accompanying tests are thorough and cover the new functionality well. I've found one critical issue that will prevent the code from compiling, which I've detailed in a specific comment.
There was a problem hiding this comment.
Pull request overview
Implements a one-phase commit path for single-group transactions, reducing transactional overhead by applying user mutations directly at a commit timestamp (without writing txn intents/locks/commit records) while keeping existing multi-phase behavior intact.
Changes:
- Build/dispatch single-group txns as a single
pb.RequestwithPhase_NONE(one-phase txn) instead of PREPARE+COMMIT. - Teach the KV FSM to treat
IsTxn && Phase_NONEas a one-phase txn and apply mutations directly (no txn artifacts). - Update cleanup/forwarding logic to account for one-phase txns and add tests to verify behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
kv/transaction.go |
Skips abort cleanup for one-phase txns; refactors abort request construction into a helper. |
kv/fsm.go |
Adds one-phase txn handling (Phase_NONE) and applies user mutations directly at commitTS. |
kv/coordinator.go |
Switches single-group txn dispatch/redirect to emit a single one-phase request. |
kv/fsm_txn_test.go |
Verifies one-phase txns commit data without leaving txn lock/intent/commit artifacts. |
kv/coordinator_txn_test.go |
Verifies coordinator emits a single Phase_NONE txn request with meta + mutations. |
adapter/internal.go / adapter/internal_test.go |
Ensures forwarded one-phase txns get a valid CommitTS assigned in txn meta. |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: bootjp <1306365+bootjp@users.noreply.github.com> Agent-Logs-Url: https://github.com/bootjp/elastickv/sessions/4df9d734-df5c-461d-a542-30fa52e1d174
Co-authored-by: bootjp <1306365+bootjp@users.noreply.github.com> Agent-Logs-Url: https://github.com/bootjp/elastickv/sessions/4df9d734-df5c-461d-a542-30fa52e1d174
[WIP] Implement one-phase commit for single-group transactions
All four Gemini findings addressed. #189 -- pendingWideColumnTTL slice bounded. The orphan-TTL buffer is now capped at maxPendingWideColumnTTL = 1,000,000 entries (~50 MiB). Records past the cap are dropped and the warn sink reports the count at Finalize. Real production state (where wide-column type encoders eventually claim every TTL) is far under the cap; the bound only protects against malformed or adversarial snapshots. #217 -- db_0 hardcoding fixed. NewRedisDB now takes a dbIndex parameter; the per-encoder root "<outRoot>/redis/db_<idx>/" is computed from it. Two encoders with the same outRoot but different indices no longer collide. TestRedisDB_PerDBIndexRoutesIntoOwnDirectory locks in the distinction. #218 -- MkdirAll cached. Added dirsCreated map[string]struct{} on RedisDB. ensureDir() checks the map before MkdirAll so repeated writes (one per blob record) collapse to a map lookup. For a 10M-key dump this saves ~10M stat+mkdir(EEXIST) round-trips. TestRedisDB_DirsCreatedCachesMkdirAll asserts the cache is populated exactly once per directory. #382 -- ENOSPC handling. Added IsBlobAtomicWriteOutOfSpace as the explicit ENOSPC probe. IsBlobAtomicWriteRetriable continues to report only io.ErrShortWrite as retriable -- ENOSPC is intentionally NOT retriable (a backup against a full disk should surface to the operator rather than spin). The two-function split lets the master pipeline render the right alarm message.
No description provided.