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.

Sign up for GitHub

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

Jump to bottom

Consider refactoring Snapshot type bound. #209

Closed
Tracked by #857
drmingdrmer opened this issue Feb 25, 2022 · 9 comments · Fixed by #1016
Closed
Tracked by #857

Consider refactoring Snapshot type bound. #209

drmingdrmer opened this issue Feb 25, 2022 · 9 comments · Fixed by #1016
Assignees
@lichuang
Labels
A-snapshot Area: snapshot creation/transport/management

Comments

@drmingdrmer
Copy link
Member

This interface is geared toward small file-based snapshots. However, not all snapshots can
be easily represented as a file. Probably a more generic interface will be needed to address
also other needs.

https://github.com/datafuselabs/openraft/blob/46644c8409c7dff627c991cc65849507ad5265b3/openraft/src/storage.rs#L147-L159
@github-actions
Copy link

👋 Thanks for opening this issue!

Get help or engage by:

  • /help : to print help messages.
  • /assignme : to assign this issue to you.

@drmingdrmer drmingdrmer added the A-snapshot Area: snapshot creation/transport/management label Feb 25, 2022
@lichuang lichuang self-assigned this Feb 28, 2022
@fredfortier
Copy link

fredfortier commented Mar 5, 2022 •
edited
Loading

I'd like to understand this a bit better. I could perhaps submit a PR to suggesting an implementation. My snapshots are actually large and not file-based. The interface works afaik, but I do experience issues like #235 that may be related to these snapshot assumptions.

@drmingdrmer
Copy link
Member Author

The API forces user to implement a snapshot in a byte-stream way.
IMHO, a snapshot would be better to be a stream of a user-defined data type.

And incremental snapshot replication may be useful.

@schreter
Copy link
Collaborator

schreter commented Mar 5, 2022

@fredfortier We also have a relatively complex snapshot that is not file based. Basically, it is a set of pages to transfer with some meta information regarding page location (since the pages may cross-reference themselves). This needs to be materialized at the remote side.

Although it is surely possible to access such a snapshot using a byte stream, it's quite complex to provide a reliable working random-access implementation to it. It would be simpler to handle it in a different way, with records with associated metadata.

I even though of sending a small snapshot info (~2KB) to the follower via the existing snapshot interface and then actively request data belonging to this snapshot from the leader as needed. I still don't have a concrete idea how to cut the API the best to handle all possible use cases.

@drmingdrmer drmingdrmer mentioned this issue Aug 2, 2022
Open
@zach-schoenberger
Copy link
Contributor

zach-schoenberger commented Nov 4, 2022 •
edited
Loading

@drmingdrmer would it make sense to make Snapshot's bounds be futures_core::Stream + Send + Unpin + 'static instead of AsyncRead + AsyncSeek + Send + Unpin + 'static for RaftSnapshotBuilder? I was going through looking how to implement the snapshoting in my project and ran into an issue with the current trait bounds. Mainly that AsyncSeek isn't really an option for me. And looking at the snapshot code i'm not really sure why it's needed. Any insight there would be great!

@drmingdrmer
Copy link
Member Author

@drmingdrmer would it make sense to make Snapshot's bounds be futures_core::Stream + Send + Unpin + 'static instead of AsyncRead + AsyncSeek + Send + Unpin + 'static for RaftSnapshotBuilder?

Agree. a Stream of user-defined data chunks would be better. And it can just be a stream of Vec<u8> to simulate an AsyncRead.

I was going through looking how to implement the snapshoting in my project and ran into an issue with the current trait bounds. Mainly that AsyncSeek isn't really an option for me. And looking at the snapshot code i'm not really sure why it's needed. Any insight there would be great!

I guess it is a historical reason: the API is derived from etcd-raft in which a snapshot is just a byte stream.

@zach-schoenberger
Copy link
Contributor

@drmingdrmer would you mind checking if the approach I took here https://github.com/zach-schoenberger/openraft/tree/snapshot_stream looks good to you? If so I'll finish out updating the tests and examples

@drmingdrmer
Copy link
Member Author

@drmingdrmer would you mind checking if the approach I took here https://github.com/zach-schoenberger/openraft/tree/snapshot_stream looks good to you? If so I'll finish out updating the tests and examples

Thank you for your suggestion.
Can you make it a draft PR so that everyone will be able to see the changes clearly online?

@zach-schoenberger
Copy link
Contributor

noting here the pr: #600

@drmingdrmer drmingdrmer mentioned this issue Jun 6, 2023
Closed
4 tasks
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Feb 19, 2024
@drmingdrmer
Feature: feature flag general-snapshot-data
655978d 
Add feature flag `general-snapshot-data`: when enabled, `SnapshotData`
does not have `AsyncSeek + AsyncRead + AsyncWrite` bound.
This enables application to define their own snapshot format and
transmission protocol.

If this feature flag is not eabled, no changes are required for
application to upgrade Openraft.

On the sending end(leader that sends snapshot to follower):

- Without `general-snapshot-data`: `RaftNetwork::snapshot()`
  provides a default implementation that invokes the chunk based API
  `RaftNetwork::install_snapshot()` for transmit.

- With `general-snapshot-data` enabled: `RaftNetwork::snapshot()` must be
  implemented to provide application customized snapshot transmission.
  Application does not also use `RaftNetwork::install_snapshot()` for

On the receiving end(follower):

- `Raft::install_snapshot()` is available only when
  `general-snapshot-data` is disabled.

Add an example `examples/raft-kv-memstore-general-snapshot-data` with
`general-snapshot-data` enabled.
In this example snapshot is transmitted without fragmentation, i.e., via
`RaftNetwork::snapshot()`. The chunk based API
`RaftNetwork::install_snapshot()` is not used.
In a production scenario, a snapshot can be transmitted in arbitrary
manner.

- Fix: databendlabs#606
- Fix: databendlabs#209
@drmingdrmer drmingdrmer mentioned this issue Feb 19, 2024
Merged
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Feb 19, 2024
@drmingdrmer
Feature: feature flag general-snapshot-data
23f77b3 
Add feature flag `general-snapshot-data`: when enabled, `SnapshotData`
does not have `AsyncSeek + AsyncRead + AsyncWrite` bound.
This enables application to define their own snapshot format and
transmission protocol.

If this feature flag is not eabled, no changes are required for
application to upgrade Openraft.

On the sending end(leader that sends snapshot to follower):

- Without `general-snapshot-data`: `RaftNetwork::snapshot()`
  provides a default implementation that invokes the chunk based API
  `RaftNetwork::install_snapshot()` for transmit.

- With `general-snapshot-data` enabled: `RaftNetwork::snapshot()` must be
  implemented to provide application customized snapshot transmission.
  Application does not need to implement `RaftNetwork::install_snapshot()`.

On the receiving end(follower):

- `Raft::install_snapshot()` is available only when
  `general-snapshot-data` is disabled.

Add an example `examples/raft-kv-memstore-general-snapshot-data` with
`general-snapshot-data` enabled.
In this example snapshot is transmitted without fragmentation, i.e., via
`RaftNetwork::snapshot()`. The chunk based API
`RaftNetwork::install_snapshot()` is not used.
In a production scenario, a snapshot can be transmitted in arbitrary
manner.

- Fix: databendlabs#606
- Fix: databendlabs#209
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Feb 19, 2024
@drmingdrmer
Feature: feature flag generic-snapshot-data
98b1dcd 
Add feature flag `generic-snapshot-data`: when enabled, `SnapshotData`
does not have `AsyncSeek + AsyncRead + AsyncWrite` bound.
This enables application to define their own snapshot format and
transmission protocol.

If this feature flag is not eabled, no changes are required for
application to upgrade Openraft.

On the sending end(leader that sends snapshot to follower):

- Without `generic-snapshot-data`: `RaftNetwork::snapshot()`
  provides a default implementation that invokes the chunk based API
  `RaftNetwork::install_snapshot()` for transmit.

- With `generic-snapshot-data` enabled: `RaftNetwork::snapshot()` must be
  implemented to provide application customized snapshot transmission.
  Application does not need to implement `RaftNetwork::install_snapshot()`.

On the receiving end(follower):

- `Raft::install_snapshot()` is available only when
  `generic-snapshot-data` is disabled.

Add an example `examples/raft-kv-memstore-generic-snapshot-data` with
`generic-snapshot-data` enabled.
In this example snapshot is transmitted without fragmentation, i.e., via
`RaftNetwork::snapshot()`. The chunk based API
`RaftNetwork::install_snapshot()` is not used.
In a production scenario, a snapshot can be transmitted in arbitrary
manner.

- Fix: databendlabs#606
- Fix: databendlabs#209
drmingdrmer added a commit that referenced this issue Feb 19, 2024
@drmingdrmer
Feature: feature flag generic-snapshot-data
907f5f7 
Add feature flag `generic-snapshot-data`: when enabled, `SnapshotData`
does not have `AsyncSeek + AsyncRead + AsyncWrite` bound.
This enables application to define their own snapshot format and
transmission protocol.

If this feature flag is not eabled, no changes are required for
application to upgrade Openraft.

On the sending end(leader that sends snapshot to follower):

- Without `generic-snapshot-data`: `RaftNetwork::snapshot()`
  provides a default implementation that invokes the chunk based API
  `RaftNetwork::install_snapshot()` for transmit.

- With `generic-snapshot-data` enabled: `RaftNetwork::snapshot()` must be
  implemented to provide application customized snapshot transmission.
  Application does not need to implement `RaftNetwork::install_snapshot()`.

On the receiving end(follower):

- `Raft::install_snapshot()` is available only when
  `generic-snapshot-data` is disabled.

Add an example `examples/raft-kv-memstore-generic-snapshot-data` with
`generic-snapshot-data` enabled.
In this example snapshot is transmitted without fragmentation, i.e., via
`RaftNetwork::snapshot()`. The chunk based API
`RaftNetwork::install_snapshot()` is not used.
In a production scenario, a snapshot can be transmitted in arbitrary
manner.

- Fix: #606
- Fix: #209
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lichuang lichuang

Labels
A-snapshot Area: snapshot creation/transport/management
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Feature: feature flag generic-snapshot-data drmingdrmer/openraft
5 participants
@drmingdrmer @fredfortier @lichuang @zach-schoenberger @schreter