Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Commitlog offset index #1671

Merged
merged 6 commits into from
Sep 24, 2024
Merged

Commitlog offset index #1671

merged 6 commits into from
Sep 24, 2024

Conversation

Shubham8287
Copy link
Contributor

Description of Changes

Implements Commitlog offset proposal -https://github.com/clockworklabs/SpacetimeDBPrivate/pull/867

Reader integration is left to do in another PR.

API and ABI breaking changes

N/A

Testing

  • unit tests, and manual run for integration testing

@Shubham8287 Shubham8287 requested a review from kim September 4, 2024 21:09
@Shubham8287 Shubham8287 force-pushed the shub/commitlog-index branch 12 times, most recently from 2a50dc3 to 982a76e Compare September 5, 2024 08:03
crates/commitlog/src/index/indexfile.rs Outdated Show resolved Hide resolved
/// - `IndexError::OutOfMemory`: Append after index file is already full.
fn append(&mut self, key: Key, value: u64) -> Result<(), IndexError>;

/// Asynchronously flushes any pending changes to the index file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to make this return io::Result (because that is, in fact, the only error which can occur).

Also worth explaining that an Ok doesn't indicate whether or not the flush succeeded, an Err result only says that it definitely did not succeed.

crates/commitlog/src/index/mod.rs Outdated Show resolved Hide resolved
}
}

pub type TxOffsetIndex = IndexFileMut<TxOffset>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a concrete type. Do you plan to support anything other than memory maps? We can also have anonymous maps, not backed by a file.

If there's no immediate plan for a different implementation, I don't think we actually need the IndexRead, IndexWrite traits.

Otherwise, the Repo trait will need an associated type type OffsetIndex: IndexRead + IndexWrite and:

fn get_offset_index(&self, offset: TxOffset, cap: u64) -> io::Result<Self::OffsetIndex>

crates/commitlog/src/repo/fs.rs Show resolved Hide resolved
crates/commitlog/src/repo/fs.rs Outdated Show resolved Hide resolved
crates/commitlog/src/index/indexfile.rs Show resolved Hide resolved
crates/commitlog/src/repo/mod.rs Outdated Show resolved Hide resolved
@@ -17,6 +18,23 @@ pub use fs::Fs;
#[cfg(test)]
pub use mem::Memory;

#[derive(Debug, Default, Clone, Copy)]
pub struct TxOffset(u64);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this not just be a type alias?
If we feel like it's useful, we should also change the other Repo methods which take an offset.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made it alias, newtype is not useful right now but may be when we use different Indexes like timestamps based.
Though, It will require bunch of mechanical change, can be done separately or when needed.

crates/commitlog/src/index/indexfile.rs Show resolved Hide resolved
@bfops bfops added the release-any To be landed in any release window label Sep 9, 2024
@Shubham8287 Shubham8287 added release-1.0 and removed release-any To be landed in any release window labels Sep 11, 2024
@Shubham8287 Shubham8287 self-assigned this Sep 11, 2024
Shubham8287 and others added 5 commits September 24, 2024 20:31
Co-authored-by: Kim Altintop <kim@eagain.io>
Signed-off-by: Shubham Mishra <shubham@clockworklabs.io>
@Shubham8287 Shubham8287 added this pull request to the merge queue Sep 24, 2024
Merged via the queue into master with commit eeaa00a Sep 24, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants