-
Notifications
You must be signed in to change notification settings - Fork 124
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
Disable checkpointing #2124
Disable checkpointing #2124
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -182,7 +182,8 @@ impl SourceChannelManager { | |
|
||
fn should_participate_in_commit(&self) -> bool { | ||
self.num_uncommitted_ops >= self.commit_sz | ||
|| self | ||
|| self.num_uncommitted_ops > 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can tweak this already with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I targeted the else branch, which is time based. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see what it does now. Makes sense 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is really subtle. Let me try to explain. To generate a The So we shouldn't add this line. The information is not lost though. |
||
&& self | ||
.last_commit_instant | ||
.elapsed() | ||
.unwrap_or(self.max_duration_between_commits) // In case of system time drift, we just commit | ||
|
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.
We might still write the checkpoints, which means this will fail to restart every time
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.
Yes, we need to fix the restart issue separately. Once we settle on a solution, I will open a pull request for it.
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 I'd prefer to solve the restart issue in this pull request as well.