-
Notifications
You must be signed in to change notification settings - Fork 37
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
[Bifrost] Watch tail updates #1743
Conversation
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.
Nice work @AhmedSoliman :-) LGTM. +1 for merging. My comment about pin-project
is unrelated.
crates/bifrost/src/loglet/util.rs
Outdated
} | ||
|
||
/// Blocks until the offset is greater or equal to the given offset. | ||
/// Blocks until the tail is beyong the given offset. |
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.
beyond
#[pin] | ||
iterator: DBRawIteratorWithThreadMode<'static, DB>, | ||
#[pin] | ||
release_watch: WatchStream<LogletOffset>, | ||
tail_watch: BoxStream<'static, TailState<LogletOffset>>, | ||
#[pin] | ||
terminated: bool, |
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 think that these fields no longer need to be pinned. That way we could get completely rid of pin_project
for this type.
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'll do that in a follow up PR.
@@ -179,9 +183,9 @@ struct MemoryReadStream { | |||
/// The next offset to read from | |||
read_pointer: LogletOffset, | |||
#[pin] |
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 think there is no need for the pin projections here.
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.
Will do that in a follow up PR on top of the stack
Loglet wrapper now limits reads if the loglet sealed tail is known
[Bifrost] Watch tail updates
Stack created with Sapling. Best reviewed with ReviewStack.