feat: support custom sequence number generators#4
Merged
Conversation
- Accept custom SequenceNumberGenerator via DatabaseBuilder - Thread SharedSequenceNumberGenerator through Supervisor, MetaKeyspace, SnapshotTracker, and recovery - Default to SequenceNumberCounter when no custom generator is set - Add integration tests: custom generator, recovery, default behavior Closes fjall-rs#174
The custom generator is used for write sequencing, not for MVCC visibility. The visibility watermark is managed internally by the snapshot tracker using a separate counter.
visible_seqno is an MVCC visibility watermark updated via publish()/set(), not a sequence generator — intentionally decoupled from the custom write seqno generator.
Replace default-only example with one that demonstrates creating and passing a custom generator via the builder API.
Assert fetch_max/set calls instead of get calls — get() may be called only for debug logging without advancing generator state.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces per-database counters with shared, Arc-wrapped sequence-number generators; exposes generator types publicly; adds Builder API to inject a custom generator; propagates generators through config, meta, snapshot, supervisor, and recovery; adds tests for custom generators; switches lsm-tree dependency to a git fork. Changes
Sequence DiagramsequenceDiagram
participant User as User Code
participant Builder as Builder (API)
participant Supervisor as Supervisor
participant Meta as MetaKeyspace
participant Recovery as Recovery Process
User->>Builder: set seqno_generator(custom_gen)
Builder->>Builder: store generator in config
Builder->>Supervisor: create/open database with config
Supervisor->>Supervisor: make_seqno_generators(config)
Supervisor->>Meta: create/open with shared generators
Note over Supervisor,Meta: seqno and visible_seqno are Arc-wrapped shared generators (rgba(100,149,237,0.5))
User->>Supervisor: write (request SeqNo)
Supervisor->>Meta: allocate SeqNo via shared seqno
User->>Recovery: trigger/open recovery
Recovery->>Supervisor: new_with_generators(seqno.clone(), snapshot_tracker)
Recovery->>Meta: restore state and call fetch_max/set on generators
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
…ator-implementation
There was a problem hiding this comment.
Pull request overview
Adds support for injecting a custom SequenceNumberGenerator into Database::builder(), threads a shared generator through core components (Supervisor, MetaKeyspace, SnapshotTracker, recovery), and verifies behavior with new integration tests.
Changes:
- Introduce optional custom seqno generator in DB config/builder, defaulting to
SequenceNumberCounter. - Replace internal
SequenceNumberCounterplumbing withSharedSequenceNumberGeneratoracross supervisor/snapshot/meta/recovery paths. - Add integration tests covering custom generator usage, recovery advancement, and default behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/custom_seqno_generator.rs | Adds integration tests validating custom generator calls, recovery advancement, and default behavior. |
| src/supervisor.rs | Switches supervisor seqno type to SharedSequenceNumberGenerator. |
| src/snapshot_tracker.rs | Makes snapshot tracker accept/return shared generators and adapts unit tests. |
| src/recovery.rs | Uses generator-aware LSM config creation during recovery. |
| src/meta_keyspace.rs | Threads shared generators through meta keyspace state. |
| src/lib.rs | Re-exports generator-related types for public API consumption. |
| src/keyspace/mod.rs | Uses generator-aware LSM config creation for keyspaces. |
| src/db_config.rs | Adds optional custom shared generator to DB config. |
| src/db.rs | Wires custom/default generators into DB open/create paths and meta tree config. |
| src/builder.rs | Adds DatabaseBuilder::seqno_generator() API and documentation. |
| src/batch/mod.rs | Imports generator trait for seqno-related operations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
…tom-sequencenumbergenerator-implementation # Conflicts: # src/recovery.rs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/recovery.rs (1)
95-101:⚠️ Potential issue | 🔴 Critical
Config::new_with_generatorsdoes not exist.Same issue as in
src/keyspace/mod.rs— this constructor is not available in the currentlsm_treeversion. The pipeline suggestsConfig::newexists as an alternative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/recovery.rs` around lines 95 - 101, The code calls lsm_tree::Config::new_with_generators which doesn't exist; replace that constructor with lsm_tree::Config::new(path) and then configure the instance with the available setter methods instead of the nonexistent constructor: start with let base_config = lsm_tree::Config::new(path) and then chain the existing .use_descriptor_table(db.config.descriptor_table.clone()) and .use_cache(db.config.cache.clone()), and if the lsm_tree API provides methods to inject the seqno generator or snapshot tracker (e.g., use_seqno_generator, use_snapshot_tracker or similarly named setters), call those with db.supervisor.seqno.clone() and db.supervisor.snapshot_tracker.get_ref(); otherwise remove the generator args and adapt to the current Config API.src/keyspace/mod.rs (1)
341-347:⚠️ Potential issue | 🔴 Critical
Config::new_with_generatorsdoes not exist inlsm_tree3.1.1 — refactor to useConfig::new.The method
new_with_generatorsis not available inlsm_tree3.1.1 (the current dependency version in Cargo.toml). The correct constructor isConfig::new(path, seqno, visible_seqno)which requires explicitSequenceNumberCounterarguments. This breaks compilation in multiple locations:
- src/keyspace/mod.rs (lines 341–347)
- src/db.rs (2 occurrences)
- src/recovery.rs (1 occurrence)
Refactor all calls to pass the required sequence number counters directly to
Config::new.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/keyspace/mod.rs` around lines 341 - 347, The code calls lsm_tree::Config::new_with_generators (e.g., where base_config is created) but lsm_tree 3.1.1 only exposes Config::new(path, seqno_counter, visible_seqno_counter); replace each use of Config::new_with_generators with Config::new and pass the appropriate SequenceNumberCounter objects directly (use db.supervisor.seqno and db.supervisor.snapshot_tracker.get_ref() or their underlying counters as required), then reapply the builder calls .use_descriptor_table(...) and .use_cache(...); update all occurrences noted (src/keyspace/mod.rs base_config, the two spots in src/db.rs, and src/recovery.rs) to construct Config via Config::new(...) with the explicit counters.
🧹 Nitpick comments (1)
tests/custom_seqno_generator.rs (1)
51-124: Rename these tests to match the repository convention.
custom_generator_basic,custom_generator_recovery, anddefault_generator_unchangeddo not follow the repo's<what>_<condition>_<expected>test naming pattern, which makes the suite less consistent to scan and grep. As per coding guidelines,Name test functions following the pattern fn <what>_<condition>_<expected>().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/custom_seqno_generator.rs` around lines 51 - 124, The test function names don't follow the repo pattern fn <what>_<condition>_<expected>(); rename the three tests to descriptive names and update any references: change custom_generator_basic to seqno_generator_custom_write_calls, custom_generator_recovery to seqno_generator_recovery_advances_state, and default_generator_unchanged to seqno_generator_default_unchanged; update the function declarations for those symbols (and any test references/imports) so the new names replace the old ones consistently in tests/custom_seqno_generator.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/batch/mod.rs`:
- Line 9: Remove the unused and incorrect import SequenceNumberGenerator from
the use line that imports from lsm_tree; either delete it entirely (since the
file calls self.db.supervisor.seqno.next() and doesn't need the type), or if a
type is required elsewhere, replace SequenceNumberGenerator with the correct
SequenceNumberCounter from lsm_tree so the import reads SequenceNumberCounter
instead of SequenceNumberGenerator.
In `@src/builder.rs`:
- Line 6: The import lists a non-existent symbol SharedSequenceNumberGenerator
from lsm_tree; remove SharedSequenceNumberGenerator from the use statement
(leaving Cache, CompressionType, DescriptorTable) and either (a) update any code
that referenced SharedSequenceNumberGenerator to use the local sequence-number
wrapper or existing generator type in this crate, or (b) add a short TODO and
conditional compile (or feature gate) to reintroduce
SharedSequenceNumberGenerator when upstream lsm-tree#265 is released; search for
references to SharedSequenceNumberGenerator in this module and replace them with
the chosen alternative.
In `@src/db_config.rs`:
- Line 6: The import lists a non-existent symbol SharedSequenceNumberGenerator
from lsm_tree; remove that identifier from the use statement (alongside Cache,
CompressionType, DescriptorTable) and either replace it with the correct
exported symbol from the lsm_tree crate (check the crate docs for the actual
sequence-number generator type/name) or remove all usages in this file (update
any references to SharedSequenceNumberGenerator in this file to the correct
type/name). Ensure the use line in src/db_config.rs is adjusted and any
structs/functions that referenced SharedSequenceNumberGenerator are updated to
use the crate's real type so the project compiles.
In `@src/keyspace/mod.rs`:
- Line 23: The import references a non-existent type SequenceNumberGenerator
from lsm_tree; remove SequenceNumberGenerator from the use line in
src/keyspace/mod.rs (lsm_tree::{AbstractTree, AnyTree, SeqNo, UserKey,
UserValue}) and either (a) update Cargo.toml to depend on the fork/branch
fjall-rs/lsm-tree#265 until that change is released so SequenceNumberGenerator
is available, or (b) replace all uses of SequenceNumberGenerator in this module
with the crate's current alternative (e.g., SeqNo-based generator or whatever
generator API exists in the released lsm_tree) and adjust calling code
accordingly.
In `@src/lib.rs`:
- Around line 172-174: The crate re-exports SequenceNumberGenerator and
SharedSequenceNumberGenerator from lsm_tree, but those symbols do not exist
upstream (only SequenceNumberCounter is available), causing compilation
failures; fix by removing the invalid re-exports (SequenceNumberGenerator,
SharedSequenceNumberGenerator) from the pub use list in src/lib.rs and either
re-export the existing SequenceNumberCounter or gate the new re-exports behind a
feature/conditional that only enables them once lsm-tree#265 is merged and
released so the public API matches what the upstream crate exports.
In `@src/recovery.rs`:
- Around line 16-17: The import list incorrectly references a non-existent type
SequenceNumberGenerator from lsm_tree and brings in Arc which is unused; remove
SequenceNumberGenerator from the use lsm_tree::... line and drop the unused Arc
import from the std::{...} import so only AbstractTree (and any actually used
std items) remain imported; if SequenceNumberGenerator is intended later, gate
that import behind the feature or add a TODO comment referencing the upstream
issue instead of importing a non-existent symbol.
In `@src/snapshot_tracker.rs`:
- Line 12: The new type SharedSequenceNumberGenerator is unreleased upstream;
revert the changes back to the previous SequenceNumberCounter type for the seqno
field, the constructor parameter, and the get_ref return type (i.e. restore
SequenceNumberCounter wherever seqno, the constructor signature, and get_ref
currently reference SharedSequenceNumberGenerator); alternatively, if you need
forward-compatibility, gate the SharedSequenceNumberGenerator changes behind a
feature/cfg and default to SequenceNumberCounter until lsm-tree#265 is
available.
In `@src/supervisor.rs`:
- Line 26: The field seqno in the Supervisor struct was changed to
SharedSequenceNumberGenerator but that type isn't exported upstream; revert the
type back to the previous SequenceNumberCounter (or the currently exported type
used by other modules) on the seqno field and restore any associated
constructor/usage to call the existing SequenceNumberCounter APIs (e.g., in
Supervisor::new and any methods referencing seqno); alternatively, if you
intended to use SharedSequenceNumberGenerator, add a clear TODO and a
compile-time cfg/feature guard around uses of SharedSequenceNumberGenerator and
its imports so the code only builds once the lsm-tree crate exports that type.
- Line 5: The code imports a non-existent type SharedSequenceNumberGenerator
from lsm_tree; replace usages and the import in src/supervisor.rs to use the
actual exported type (e.g., lsm_tree::SequenceNumberGenerator) or introduce a
local wrapper type (e.g., Arc<Mutex<SequenceNumberGenerator>> or a small
SharedSequenceNumberGenerator wrapper) matching how the rest of the code expects
shared semantics, and update all references to SharedSequenceNumberGenerator to
that real type; if you really need the upstream change, instead pin Cargo.toml
to the fork/commit that contains lsm-tree#265 and add a comment explaining the
temporary pin until the upstream release.
In `@tests/custom_seqno_generator.rs`:
- Around line 55-68: The test should snapshot the generator count immediately
after creating the keyspace to avoid counting metadata writes from keyspace
creation: after calling db.keyspace(...) read
generator.next_count.load(Ordering::Relaxed) into a baseline variable, perform
the two tree.insert(...) calls, then assert that
generator.next_count.load(Ordering::Relaxed) - baseline >= 2; update the
assertion that currently checks generator.next_count directly (referencing
TrackingGenerator, Database::builder().seqno_generator, db.keyspace,
generator.next_count, and tree.insert).
---
Outside diff comments:
In `@src/keyspace/mod.rs`:
- Around line 341-347: The code calls lsm_tree::Config::new_with_generators
(e.g., where base_config is created) but lsm_tree 3.1.1 only exposes
Config::new(path, seqno_counter, visible_seqno_counter); replace each use of
Config::new_with_generators with Config::new and pass the appropriate
SequenceNumberCounter objects directly (use db.supervisor.seqno and
db.supervisor.snapshot_tracker.get_ref() or their underlying counters as
required), then reapply the builder calls .use_descriptor_table(...) and
.use_cache(...); update all occurrences noted (src/keyspace/mod.rs base_config,
the two spots in src/db.rs, and src/recovery.rs) to construct Config via
Config::new(...) with the explicit counters.
In `@src/recovery.rs`:
- Around line 95-101: The code calls lsm_tree::Config::new_with_generators which
doesn't exist; replace that constructor with lsm_tree::Config::new(path) and
then configure the instance with the available setter methods instead of the
nonexistent constructor: start with let base_config =
lsm_tree::Config::new(path) and then chain the existing
.use_descriptor_table(db.config.descriptor_table.clone()) and
.use_cache(db.config.cache.clone()), and if the lsm_tree API provides methods to
inject the seqno generator or snapshot tracker (e.g., use_seqno_generator,
use_snapshot_tracker or similarly named setters), call those with
db.supervisor.seqno.clone() and db.supervisor.snapshot_tracker.get_ref();
otherwise remove the generator args and adapt to the current Config API.
---
Nitpick comments:
In `@tests/custom_seqno_generator.rs`:
- Around line 51-124: The test function names don't follow the repo pattern fn
<what>_<condition>_<expected>(); rename the three tests to descriptive names and
update any references: change custom_generator_basic to
seqno_generator_custom_write_calls, custom_generator_recovery to
seqno_generator_recovery_advances_state, and default_generator_unchanged to
seqno_generator_default_unchanged; update the function declarations for those
symbols (and any test references/imports) so the new names replace the old ones
consistently in tests/custom_seqno_generator.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ddbebb6c-06ff-4bc4-aca3-fd886823bb9b
📒 Files selected for processing (11)
src/batch/mod.rssrc/builder.rssrc/db.rssrc/db_config.rssrc/keyspace/mod.rssrc/lib.rssrc/meta_keyspace.rssrc/recovery.rssrc/snapshot_tracker.rssrc/supervisor.rstests/custom_seqno_generator.rs
…ator-implementation
…ator-implementation
- Remove unused SequenceNumberGenerator imports across 6 files - Remove unused Arc import from recovery.rs - Extract make_seqno_generators() to deduplicate recover/create_new - Capture test baseline after keyspace creation for accurate assertion - Add doc comments for fetch_add semantics and API design choices Relates to #4
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Member
Author
Addressing outside-diff-range comments from CodeRabbit review
These are stale. The Cargo.toml dependency was switched from crates.io |
There was a problem hiding this comment.
Pull request overview
Adds support for plugging in a custom sequence-number generator at DB build time, and threads the shared generator through core components (supervisor/meta keyspace/snapshot tracking) and recovery so sequence numbers remain consistent across restarts.
Changes:
- Add
DatabaseBuilder::seqno_generator()to accept aSharedSequenceNumberGenerator, defaulting toSequenceNumberCounterwhen unset. - Thread
SharedSequenceNumberGeneratorthroughSupervisor,MetaKeyspace,SnapshotTracker, and LSM-tree config creation (new_with_generators) in both create/recover paths. - Add integration tests covering custom generator usage, recovery behavior, and ensuring default behavior remains unchanged.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/custom_seqno_generator.rs | New integration tests for custom generator invocation + recovery + default behavior. |
| src/supervisor.rs | Switch supervisor seqno field to shared generator type. |
| src/snapshot_tracker.rs | Accept/store shared generator for visibility watermark; update unit tests accordingly. |
| src/recovery.rs | Use Config::new_with_generators and pass shared generators during keyspace recovery. |
| src/meta_keyspace.rs | Store shared generators for meta keyspace operations and visibility advancement. |
| src/lib.rs | Re-export generator types/traits from lsm_tree as part of fjall’s public API. |
| src/keyspace/mod.rs | Use Config::new_with_generators when creating new keyspaces. |
| src/db_config.rs | Add optional custom generator to DB config. |
| src/db.rs | Add make_seqno_generators() helper; use shared generator in create/recover; keep visibility watermark separate. |
| src/builder.rs | Add seqno_generator() builder API and docs. |
| Cargo.toml | Switch lsm-tree dependency to git main to access generator trait/API. |
You can also share your feedback on Copilot code review. Take the survey.
- Doc example imports from fjall:: instead of lsm_tree:: - Doc links use intra-crate paths instead of lsm_tree:: prefix - Annotate fork git dependency in Cargo.toml Relates to #4
There was a problem hiding this comment.
Pull request overview
Adds support for injecting a custom sequence number generator into fjall via the DatabaseBuilder, and threads that generator through the database lifecycle (creation, recovery, supervisor/snapshot tracking) so write sequencing can be customized while keeping MVCC visibility tracking internal.
Changes:
- Add
DatabaseBuilder::seqno_generator()and persist the chosen generator inConfig, defaulting toSequenceNumberCounterwhen unset. - Thread
SharedSequenceNumberGeneratorthrough Supervisor, SnapshotTracker, MetaKeyspace, keyspace creation, and recovery (vialsm_tree::Config::new_with_generators). - Add integration tests covering custom generator usage, recovery behavior, and unchanged default behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/custom_seqno_generator.rs |
Integration coverage for custom generator usage, recovery behavior, and default behavior. |
src/builder.rs |
Adds public builder API to set a custom shared seqno generator. |
src/db_config.rs |
Stores optional custom generator in Config for use during open/recover. |
src/db.rs |
Creates seqno/visibility generators via helper; threads generator through DB init and recovery. |
src/supervisor.rs |
Switches supervisor seqno type to SharedSequenceNumberGenerator. |
src/snapshot_tracker.rs |
Switches snapshot tracker seqno type to shared generator; adapts tests accordingly. |
src/meta_keyspace.rs |
Switches meta keyspace seqno/visible seqno fields to shared generators. |
src/keyspace/mod.rs |
Uses lsm_tree::Config::new_with_generators when creating keyspaces. |
src/recovery.rs |
Uses lsm_tree::Config::new_with_generators when recovering keyspaces. |
src/lib.rs |
Re-exports generator-related types from lsm_tree for public use. |
Cargo.toml |
Switches lsm-tree dependency to a git fork/branch to obtain generator trait support. |
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Cargo.toml (1)
29-32: Consider pinning to a specific commit for reproducible builds.Using
branch = "main"means builds may change unexpectedly as the fork evolves. The PR summary mentions commit251a7d0was used — pinning to that would ensure reproducibility:-lsm-tree = { git = "https://github.com/structured-world/lsm-tree.git", branch = "main", default-features = false, features = [] } +lsm-tree = { git = "https://github.com/structured-world/lsm-tree.git", rev = "251a7d0", default-features = false, features = [] }This is optional since you're in alpha and plan to switch back to crates.io once upstream merges.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 29 - 32, The dependency currently pins lsm-tree to branch "main" which makes builds non-deterministic; change the lsm-tree entry to reference the exact commit used (e.g., set rev = "251a7d0" for the clone) instead of branch = "main" so the Cargo.toml lsm-tree dependency (lsm-tree = { git = "...", ... }) points to the specific commit hash used in testing for reproducible builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Cargo.toml`:
- Around line 29-32: The dependency currently pins lsm-tree to branch "main"
which makes builds non-deterministic; change the lsm-tree entry to reference the
exact commit used (e.g., set rev = "251a7d0" for the clone) instead of branch =
"main" so the Cargo.toml lsm-tree dependency (lsm-tree = { git = "...", ... })
points to the specific commit hash used in testing for reproducible builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6124c194-1c01-4cc3-9e6c-cf18f358034a
📒 Files selected for processing (9)
Cargo.tomlsrc/builder.rssrc/db.rssrc/db_config.rssrc/keyspace/mod.rssrc/meta_keyspace.rssrc/recovery.rssrc/snapshot_tracker.rstests/custom_seqno_generator.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/snapshot_tracker.rs
- tests/custom_seqno_generator.rs
- src/builder.rs
There was a problem hiding this comment.
Pull request overview
This PR adds support for injecting a custom sequence number generator into fjall via the database builder, and threads that generator through core components (supervisor, snapshot tracking, keyspace creation, and recovery) while preserving the existing default behavior.
Changes:
- Add
DatabaseBuilder::seqno_generator()to accept a shared customSequenceNumberGenerator(defaulting toSequenceNumberCounterwhen unset). - Thread
SharedSequenceNumberGeneratorthroughSupervisor,MetaKeyspace,SnapshotTracker, keyspace creation, and recovery; deduplicate init logic viaDatabase::make_seqno_generators(). - Add integration tests covering custom generator invocation, recovery behavior, and unchanged default behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/custom_seqno_generator.rs |
New integration tests verifying custom generator use and recovery behavior. |
src/supervisor.rs |
Replace concrete counter with shared generator in supervisor state. |
src/snapshot_tracker.rs |
Generalize snapshot tracker to use shared generator; update unit tests accordingly. |
src/recovery.rs |
Ensure recovered keyspaces use generator-aware LSM config initialization. |
src/meta_keyspace.rs |
Generalize meta keyspace sequencing/watermark counters to shared generators. |
src/lib.rs |
Re-export sequence generator types/traits for public API use. |
src/keyspace/mod.rs |
Ensure new keyspaces are created with generator-aware LSM config. |
src/db_config.rs |
Add optional custom generator to DB config (internal field). |
src/db.rs |
Add make_seqno_generators() and thread generators through create/recover paths. |
src/builder.rs |
Add seqno_generator() builder API with documentation and example. |
Cargo.toml |
Switch lsm-tree dependency to a git fork/branch to pick up generator trait support. |
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Cargo.toml (1)
29-32: Git dependency blocks crates.io publishing — consider adding a tracking issue.The comment clearly documents the temporary nature of this fork dependency, which is good. Since this crate cannot be published to crates.io until switching back to the official release, consider:
- Creating a GitHub issue to track when
fjall-rs/lsm-tree#265is released so you can promptly update.- Adding a
# TODO(issue-link):reference in the comment for discoverability.This is a minor suggestion for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 29 - 32, Add a tracking GitHub issue and annotate the Cargo.toml comment for the temporary lsm-tree git dependency so we remember to switch back when fjall-rs/lsm-tree#265 is released: create a repo issue referencing fjall-rs/lsm-tree#265 and the current rev (25ffae70), then update the existing comment on the lsm-tree dependency to include a TODO with the issue link (e.g., “# TODO(issue-1234): switch back to crates.io when fjall-rs/lsm-tree#265 is released”) so the dependency line for lsm-tree can be discovered and updated promptly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Cargo.toml`:
- Around line 29-32: Add a tracking GitHub issue and annotate the Cargo.toml
comment for the temporary lsm-tree git dependency so we remember to switch back
when fjall-rs/lsm-tree#265 is released: create a repo issue referencing
fjall-rs/lsm-tree#265 and the current rev (25ffae70), then update the existing
comment on the lsm-tree dependency to include a TODO with the issue link (e.g.,
“# TODO(issue-1234): switch back to crates.io when fjall-rs/lsm-tree#265 is
released”) so the dependency line for lsm-tree can be discovered and updated
promptly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 29-30: The file contains a duplicated comment line "# Fork with
SequenceNumberGenerator trait (structured-world/lsm-tree#10)." — remove one of
the two identical lines so the comment appears only once; search for the exact
string "# Fork with SequenceNumberGenerator trait
(structured-world/lsm-tree#10)." and delete the redundant occurrence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77d2e9f4-542d-42ee-a911-24410a7bd40c
📒 Files selected for processing (2)
.github/copilot-instructions.mdCargo.toml
There was a problem hiding this comment.
Pull request overview
This PR adds support for user-supplied sequence number generators, threading a shared generator through database open/create, supervisor, keyspace creation/recovery, snapshot tracking, and meta keyspace operations, while keeping the default counter-based behavior unchanged.
Changes:
- Add
DatabaseBuilder::seqno_generator()and plumbSharedSequenceNumberGeneratorthroughout DB lifecycle (create, recover, keyspace open/create). - Separate “write seqno” generation from an internal MVCC visibility watermark generator via a shared helper (
make_seqno_generators()). - Add integration tests covering custom generator usage, recovery behavior, and default behavior; switch
lsm-treeto the structured-world git fork/branch.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/custom_seqno_generator.rs | New integration tests validating custom generator call paths + recovery and default behavior. |
| src/supervisor.rs | Swap supervisor seqno type to SharedSequenceNumberGenerator. |
| src/snapshot_tracker.rs | Track MVCC watermark via shared generator; update tests for new type. |
| src/recovery.rs | Use lsm_tree::Config::new_with_generators so recovered trees share generators. |
| src/meta_keyspace.rs | Store shared generators for meta keyspace operations and visibility advancement. |
| src/lib.rs | Re-export sequence generator types/traits from lsm-tree. |
| src/keyspace/mod.rs | Use Config::new_with_generators when creating keyspaces. |
| src/db_config.rs | Add optional seqno_generator to DB config. |
| src/db.rs | Introduce make_seqno_generators() and thread generators through create/recover/meta tree init. |
| src/builder.rs | Add seqno_generator() builder API + docs. |
| Cargo.toml | Switch lsm-tree dependency to structured-world git fork (main branch). |
| .github/copilot-instructions.md | Document the intentional git dependency policy for lsm-tree. |
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Adds first-class support for plugging in a custom sequence number generator into fjall’s database construction, and threads the shared generator through the components that need consistent write sequencing and MVCC visibility tracking (including recovery).
Changes:
- Introduces
DatabaseBuilder::seqno_generator()to accept aSharedSequenceNumberGenerator, defaulting toSequenceNumberCounterwhen unset. - Propagates shared generators through
Supervisor,MetaKeyspace,SnapshotTracker, keyspace creation, and recovery vialsm_tree::Config::new_with_generators. - Adds integration tests covering custom generator usage, recovery behavior, and default behavior parity.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/custom_seqno_generator.rs | Integration tests validating custom generator calls, recovery integration, and default behavior. |
| src/supervisor.rs | Switches supervisor seqno field to SharedSequenceNumberGenerator. |
| src/snapshot_tracker.rs | Updates tracker to use shared generator type; adapts unit tests accordingly. |
| src/recovery.rs | Uses new_with_generators to ensure recovered keyspaces use shared seqno/visibility generators. |
| src/meta_keyspace.rs | Stores shared seqno + visibility generators for meta operations. |
| src/lib.rs | Re-exports sequence generator types/traits for public consumption. |
| src/keyspace/mod.rs | Uses new_with_generators for new keyspaces (write + visibility generators). |
| src/db_config.rs | Adds optional custom seqno generator to DB config. |
| src/db.rs | Adds make_seqno_generators() and wires generators through create/recover paths. |
| src/builder.rs | Adds builder API + docs for configuring the custom generator. |
| Cargo.toml | Switches lsm-tree dependency to the structured-world git fork/branch that provides the trait. |
| .github/copilot-instructions.md | Documents the intentional dependency policy around lsm-tree git tracking. |
You can also share your feedback on Copilot code review. Take the survey.
polaz
deleted the
feat/#174-custom-sequencenumbergenerator-implementation
branch
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.
Summary
SequenceNumberGeneratorviaDatabaseBuilder::seqno_generator()SharedSequenceNumberGeneratorthrough Supervisor, MetaKeyspace, SnapshotTracker, and recoverySequenceNumberCounterwhen no custom generator is setmake_seqno_generators()helper to deduplicate recover/create_new pathsUses structured-world/lsm-tree fork (main branch) which includes the SequenceNumberGenerator trait from fjall-rs/lsm-tree#265.
Test plan
TrackingGeneratortest verifies.next()is called for writes (with baseline capture).fetch_max()/.get()called during recoveryCloses fjall-rs#174
Upstream
Submitted as fjall-rs/fjall#269.
Summary by CodeRabbit
New Features
Behavioral Changes
Tests
Chores
Documentation