-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[ENH] wal3 is the write-ahead logging lightweight library #3028
base: main
Are you sure you want to change the base?
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
- A reader writing a _new_ cursor, or a cursor that goes back in time must complete the operation in | ||
less than the garbage collection interval and then check for a concurrent garbage collection | ||
before it considers the operation complete. If the reader somehow hangs between loading a log | ||
offset and writing the cursor for more than the garbage collection interval, the cursor will | ||
reference garbage collected data. The reader will fail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: reader abstraction (and cursor) has not been introduced so far
use crate::{Error, LogPosition}; | ||
|
||
////////////////////////////////////////////// Cursor ////////////////////////////////////////////// | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note for reviewer: the read path is WIP
// allows. If we hit throttle errors at this throughput we have a case for support. | ||
throughput: 2_000, | ||
// How much headroom we have for retries. | ||
headroom: 1_500, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if throughput + headroom = <s3_throughput>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's how I picked these constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case the comments seems suspicious: if a throughput of 2000 is 5/7 of max throughput, doesn't this imply the head room is 800?
.map(|f| f.seq_no) | ||
.max() | ||
.unwrap_or(ShardSeqNo(0)); | ||
max_seq_no < max_seq_no + 1 && max_seq_no + 1 == *seq_no && *start < *limit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I ask what is max_seq_no < max_seq_no + 1
checking for here? Is this equivalent to max_seq_no < u64::Max
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That check would do it, too, but precludes allowing u64::MAX
as a value. I'm fine to throw that value away. Would you find the check more readable that way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine both way. I'm wondering if it is ever possible to violate this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting @HammadB for further review
Documented in
rust/wal3/README.md
.