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

feat: Restore record store, processors and logs from checkpoints #1984

Merged
merged 7 commits into from
Sep 7, 2023
Merged

feat: Restore record store, processors and logs from checkpoints #1984

merged 7 commits into from
Sep 7, 2023

Conversation

chubei
Copy link
Contributor

@chubei chubei commented Sep 5, 2023

This is the followup of #1854.

In this PR, in addition to writing checkpoints of the record store, we restore the record store and log from the checkpoints.

We also write checkpoints of processors. Now every processor implements serialize to dump its state to an Object. Upon restart, the ProcessorFactory::build gets an additional parameter checkpoint_data, which is the data that was dumped during one of the serializes. Every processor also restores its state from checkpoint_data.

However, due to the fact that ConnectorSource currently can't start from a checkpoint, the processor restoring functions won't actually run. We'll address the connector restart problem in another PR.

Caveats:

- Didn't start source from checkpoint
- Didn't checkpoint and restore processors
- Didn't handle unpersisted cache data
@chubei
Copy link
Contributor Author

chubei commented Sep 5, 2023

I pushed another commit with a bug fix but Github has trouble verifying it. main...chubei:dozer:feat/checkpoint_restore

@Jesse-Bakker
Copy link
Contributor

Cursor seems like a re-implementation of the Buf trait from bytes. We might just want to use bytes. It is already in our dependency tree anyway.

Furthermore, I think a lot of the serialize implementations should probably be impl Serialize instead. Object can implement Write, so we can just bincode::serialize_into into it.

dozer-core/src/checkpoint/mod.rs Show resolved Hide resolved
dozer-core/src/checkpoint/mod.rs Outdated Show resolved Hide resolved
dozer-log/src/storage/local.rs Outdated Show resolved Hide resolved
dozer-log/src/storage/local.rs Outdated Show resolved Hide resolved
@chubei
Copy link
Contributor Author

chubei commented Sep 6, 2023

Buf looks like overkill? I just want one consume method and it's not even in Buf because it can be composed of multiple chunks.

I'm not using serde because the serialization of ProcessorRecord needs access to an extra parameter ProcessorRecrodStore. Again not expert on serde so don't know how to represent that.
I'm using serde and bincode (see serialize_bincode) whenever possible.

@Jesse-Bakker
Copy link
Contributor

Jesse-Bakker commented Sep 6, 2023

Buf looks like overkill? I just want one consume method and it's not even in Buf because it can be composed of multiple chunks.

I meant that it also implements most of the deserialize_<type> functions, like Buf::get_u64, and it can also supply a Read, so serde can deserialize from it. It does panic if there are not enough bytes in the buffer, however, so that probably makes it unsuitable for us.

Copy link
Contributor

@Jesse-Bakker Jesse-Bakker left a comment

Choose a reason for hiding this comment

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

Generally looks very good. I think putting effort in using serde better is not a good use of our time right now, and the current implementation looks correct and performant.

@chubei chubei added this pull request to the merge queue Sep 7, 2023
Merged via the queue into getdozer:main with commit 91b50d1 Sep 7, 2023
9 checks passed
@chubei chubei deleted the feat/checkpoint_restore branch September 7, 2023 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants