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

(2/3) Commitlog: Add I/O based on regular files #920

Merged
merged 1 commit into from
Apr 2, 2024
Merged

Conversation

kim
Copy link
Contributor

@kim kim commented Mar 4, 2024

Provides a commitlog backing store based on files, and defines the exported Commitlog type which fixes the store to the file-based one.

NOTE: This PR is stacked on top of #919

Expected complexity level and risk

1

/// visible to the iterator. Upon encountering [`io::ErrorKind::UnexpectedEof`],
/// however, a new iterator should be created using [`Self::commits_from`]
/// with the last transaction offset yielded.
pub fn commits(&self) -> impl Iterator<Item = Result<Commit, error::Traversal>> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

slash-slash-todo: We're skipping bad commits in case the next commit is good. It's not obvious what to do when the iterator is exhausted and the last commit is bad -- there could be more coming at a later point in time. So for now, we'll yield the error.

This is fine in tooling, but requires special-casing when replaying the log to fold it into a datastore. Maybe a named iterator which can be turned into one which just ignores the error if it's the last item could be handy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I settled on yielding the error in iterators, but ignore it in folds (because the consumer of a fold doesn't have access to the traversal itself).

Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

Looks great! This does exactly what I expected it to do based on the previous PR and the proposal. I suggested a few minor comments in fs.rs.

crates/commitlog/src/repo/fs.rs Show resolved Hide resolved
crates/commitlog/src/repo/fs.rs Show resolved Hide resolved
crates/commitlog/src/repo/fs.rs Show resolved Hide resolved
crates/commitlog/src/repo/fs.rs Show resolved Hide resolved
crates/commitlog/src/repo/fs.rs Show resolved Hide resolved
Copy link
Contributor

@kazimuth kazimuth left a comment

Choose a reason for hiding this comment

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

Nice and straightforward.

In the future it might be nice to try to run the existing tests against the file system Repo implementation as well. I don't think we need to do that right now though.

@bfops bfops added release-any To be landed in any release window no runtime change This change does not affect the final binaries commit-log labels Mar 18, 2024
use super::Repo;

const SEGMENT_FILE_EXT: &str = ".stdb.log";

Copy link
Contributor

Choose a reason for hiding this comment

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

.log extension could be confused easily as being text-based, how about .log.stdb or .stdb.wbl (write behind log)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file extension was requested by @Centril. If we don’t like it, an amendment to the spec shall be proposed.

Copy link
Contributor

Choose a reason for hiding this comment

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

.log is pretty standard. I think it's the same for kafka IIRC.

Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

Seems fine given the previous PR.

@kim kim force-pushed the kim/commitlog2-io branch 2 times, most recently from 5f6b3e3 to f2e3f7d Compare March 28, 2024 19:37
@kim kim changed the base branch from kim/commitlog2-sansio to master April 2, 2024 06:42
Provides a commitlog backing store based on files, and defines the
exported `Commitlog` type which fixes the store to the file-based one.
@kim kim enabled auto-merge April 2, 2024 08:35
@kim kim added this pull request to the merge queue Apr 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 2, 2024
@kim kim added this pull request to the merge queue Apr 2, 2024
Merged via the queue into master with commit 73cd782 Apr 2, 2024
6 checks passed
@kim kim deleted the kim/commitlog2-io branch April 2, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no runtime change This change does not affect the final binaries release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants