-
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] Bifrost appends will wait for reconfiguration #1772
Conversation
95bd203
to
68b23bc
Compare
33b0f5d
to
a8f4928
Compare
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.
Really good work @AhmedSoliman :-) LGTM. +1 for merging.
let start = Instant::now(); | ||
for sleep_dur in retry_iter.by_ref() { | ||
self.fail_if_shutting_down()?; | ||
tokio::time::sleep(sleep_dur).await; |
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.
Could we also wait for a logs
metadata update to occur?
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 attempted to do that but it ended up being quite tricky and prone to racy select(). I'm leaning towards leaving it as is to avoid complexity until we find a good reason to avoid the short sleep.
Bifrost appends will wait for reconfiguration to complete. Additionally, log metadata will now handle empty segments correctly. An empty segment will be removed from the chain and replaced with the newly created one but with a higher segment index.
[Bifrost] Bifrost appends will wait for reconfiguration
Bifrost appends will wait for reconfiguration to complete. Additionally, log metadata will now handle empty segments correctly. An empty segment will be removed from the chain and replaced with the newly created one but with a higher segment index.
Stack created with Sapling. Best reviewed with ReviewStack.