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

Move log segment into separate module #438

Merged
merged 23 commits into from
Nov 8, 2024

Conversation

OussamaSaoudi-db
Copy link
Collaborator

@OussamaSaoudi-db OussamaSaoudi-db commented Oct 28, 2024

What changes are proposed in this pull request?

This PR is a prefactor that moves LogSegment into its own module. This will be useful to implement TableChanges for change data feed. TableChanges will represent CDF scans and will depend on LogSegment. Since TableChanges and Snapshot both depend on LogSegment, it makes sense to keep it separated.

I also use this opportunity to address some nits in the codebase.

How was this change tested?

The changes compile.

Log segment is going to be used by both snapshot and table changes.
It makes sense to separate it into its own module
@github-actions github-actions bot added the breaking-change Change that will require a version bump label Oct 28, 2024
@OussamaSaoudi-db OussamaSaoudi-db changed the title Move log segment into separate module [WIP] Move log segment into separate module Oct 28, 2024
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 89.16667% with 13 lines in your changes missing coverage. Please review.

Project coverage is 78.95%. Comparing base (22b9e33) to head (76711f4).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
kernel/src/scan/mod.rs 75.00% 3 Missing and 4 partials ⚠️
kernel/src/log_segment.rs 93.40% 1 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #438      +/-   ##
==========================================
- Coverage   78.96%   78.95%   -0.01%     
==========================================
  Files          55       56       +1     
  Lines       12226    12216      -10     
  Branches    12226    12216      -10     
==========================================
- Hits         9654     9645       -9     
- Misses       2050     2052       +2     
+ Partials      522      519       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@OussamaSaoudi-db OussamaSaoudi-db changed the title [WIP] Move log segment into separate module Move log segment into separate module Nov 6, 2024
@OussamaSaoudi-db OussamaSaoudi-db marked this pull request as ready for review November 6, 2024 20:11
@OussamaSaoudi-db OussamaSaoudi-db requested review from zachschuermann, scovich and nicklan and removed request for zachschuermann and scovich November 6, 2024 20:11
kernel/src/log_segment.rs Outdated Show resolved Hide resolved
kernel/src/log_segment.rs Show resolved Hide resolved
kernel/src/log_segment.rs Outdated Show resolved Hide resolved
kernel/src/log_segment.rs Outdated Show resolved Hide resolved
kernel/src/log_segment.rs Outdated Show resolved Hide resolved
Comment on lines 83 to 93
if metadata_opt.is_some() && protocol_opt.is_some() {
// we've found both, we can stop
break;
}
}
match (metadata_opt, protocol_opt) {
(Some(m), Some(p)) => Ok((m, p)),
(None, Some(_)) => Err(Error::MissingMetadata),
(Some(_), None) => Err(Error::MissingProtocol),
_ => Err(Error::MissingMetadataAndProtocol),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've always wondered why not do:

Suggested change
if metadata_opt.is_some() && protocol_opt.is_some() {
// we've found both, we can stop
break;
}
}
match (metadata_opt, protocol_opt) {
(Some(m), Some(p)) => Ok((m, p)),
(None, Some(_)) => Err(Error::MissingMetadata),
(Some(_), None) => Err(Error::MissingProtocol),
_ => Err(Error::MissingMetadataAndProtocol),
}
if let (Some(m), Some(p)) = (metadata_opt, protocol_opt) {
return Ok((m, p))
}
}
match (metadata_opt, protocol_opt) {
(_, Some(_)) => Err(Error::MissingMetadata),
(Some(_), _) => Err(Error::MissingProtocol),
_ => Err(Error::MissingMetadataAndProtocol),
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This moves metadata_opt and protocol_opt early in the if let. So they're dropped before the match. It's weird because it shouldn't move if the let doesn't match :/

Copy link
Collaborator

@scovich scovich Nov 8, 2024

Choose a reason for hiding this comment

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

Yikes. I also wouldn't have expected a non-matching if let to move??

kernel/src/log_segment.rs Outdated Show resolved Hide resolved
kernel/src/scan/mod.rs Outdated Show resolved Hide resolved
kernel/src/log_segment.rs Show resolved Hide resolved
kernel/src/log_segment.rs Outdated Show resolved Hide resolved
kernel/src/log_segment.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@zachschuermann zachschuermann left a comment

Choose a reason for hiding this comment

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

LGTM but question on visibility of the new module and can we call out breaking change in PR description?

kernel/src/snapshot.rs Show resolved Hide resolved
kernel/src/lib.rs Show resolved Hide resolved
@OussamaSaoudi-db OussamaSaoudi-db merged commit 534a4e4 into delta-io:main Nov 8, 2024
17 checks passed
@OussamaSaoudi-db OussamaSaoudi-db deleted the snapshot_cleanup branch November 8, 2024 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Change that will require a version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants