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

Feature: feature flag generic-snapshot-data #1016

Merged

Conversation

drmingdrmer
Copy link
Member

@drmingdrmer drmingdrmer commented Feb 19, 2024

Changelog

Feature: feature flag generic-snapshot-data

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.

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.


This change is Reviewable

@drmingdrmer drmingdrmer force-pushed the 35-loosen-snapshot-data-trait-bound branch from 636e010 to 718fef7 Compare February 19, 2024 07:25
@drmingdrmer drmingdrmer force-pushed the 35-loosen-snapshot-data-trait-bound branch 2 times, most recently from 655978d to 23f77b3 Compare February 19, 2024 07:43
schreter
schreter previously approved these changes Feb 19, 2024
Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

general-snapshot-data

I'd suggest generic-snapshot-data, that fits it better, I think.

I didn't read all the code, but the general direction seems to fit. It will show whether there are issues once it's actually used in our code :-).

When it's merged, I'll try to integrate it with our snapshot soon (if I find time - I have to prepare a presentation and demo to our management in the next two weeks; but at the latest some time after that).

Reviewed 22 of 26 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ariesdevil, @drmingdrmer, and @lichuang)


openraft/Cargo.toml line 97 at r2 (raw file):

# Enable this feature flag to eliminate the `AsyncRead + AsyncWrite + AsyncSeek + Unpin` bound from `RaftTypeConfig::SnapshotData`.
# Enabling this feature allows applications to use a custom snapshot data format and transport fragmentation, diverging from the default implementation which typically relies on a single-file structure .

Maybe reflow the comment? The lines are quite long.

Code quote:

# Enable this feature flag to eliminate the `AsyncRead + AsyncWrite + AsyncSeek + Unpin` bound from `RaftTypeConfig::SnapshotData`.
# Enabling this feature allows applications to use a custom snapshot data format and transport fragmentation, diverging from the default implementation which typically relies on a single-file structure .

openraft/src/docs/feature_flags/feature-flags.md line 0 at r2 (raw file):
Also here, the lines are quite long.

BTW, we use dprint (via cargo dprint and also as a VS Code plugin to format on save) to format and reflow .md files to 100 chars (doesn't work for Rust comments, unfortunately).

What IDE are you using? Or just a text-based editor?

Copy link
Member Author

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Make sense!

Look forward to hearing from you.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ariesdevil, @lichuang, and @schreter)


openraft/src/docs/feature_flags/feature-flags.md line at r2 (raw file):

Previously, schreter wrote…

Also here, the lines are quite long.

BTW, we use dprint (via cargo dprint and also as a VS Code plugin to format on save) to format and reflow .md files to 100 chars (doesn't work for Rust comments, unfortunately).

What IDE are you using? Or just a text-based editor?

I use both vim and jetbrains Clion.

In vim, in normal mode gq wraps current line and in visual mode gq wraps selected lines. This works fine in most scenario. gq is syntax sensitive, it handles comment head such as # or // correctly.

In vim :help gq explains it:
image.png

In Clion, the action I bind to wrap line is Apply Structural Wrapping, which wraps rust doc comment correctly.
image copy 1.png

You can bind this action to a keystroke in Clion Keymap setting, or(as I do) bind it to an ideaVim(enabling vim functions in Clion) keystroke in .ideavimrc:

vmap Q     <Action>(Grazie.Pro.Structural.Wrap)

With this setting, in Clion press Q after selecting several lines of comment, it will wrap and reformat it.

@drmingdrmer drmingdrmer force-pushed the 35-loosen-snapshot-data-trait-bound branch from 1c5396b to 3d4b78d Compare February 19, 2024 09:54
Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

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

Reviewed 21 of 21 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ariesdevil, @drmingdrmer, and @lichuang)


openraft/src/docs/feature_flags/feature-flags.md line at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

I use both vim and jetbrains Clion.

In vim, in normal mode gq wraps current line and in visual mode gq wraps selected lines. This works fine in most scenario. gq is syntax sensitive, it handles comment head such as # or // correctly.

In vim :help gq explains it:
image.png

In Clion, the action I bind to wrap line is Apply Structural Wrapping, which wraps rust doc comment correctly.
image copy 1.png

You can bind this action to a keystroke in Clion Keymap setting, or(as I do) bind it to an ideaVim(enabling vim functions in Clion) keystroke in .ideavimrc:

vmap Q     <Action>(Grazie.Pro.Structural.Wrap)

With this setting, in Clion press Q after selecting several lines of comment, it will wrap and reformat it.

Well, we are not allowed to use Clion at SAP, but VS Code works also quite well for Rust development (with rust-analyzer).

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 drmingdrmer force-pushed the 35-loosen-snapshot-data-trait-bound branch from 3d4b78d to 98b1dcd Compare February 19, 2024 12:28
@drmingdrmer drmingdrmer changed the title Feature: feature flag general-snapshot-data Feature: feature flag generic-snapshot-data Feb 19, 2024
@drmingdrmer drmingdrmer merged commit 907f5f7 into databendlabs:main Feb 19, 2024
30 of 31 checks passed
@drmingdrmer drmingdrmer deleted the 35-loosen-snapshot-data-trait-bound branch February 19, 2024 12:56
@drmingdrmer drmingdrmer mentioned this pull request Feb 19, 2024
3 tasks
@drmingdrmer drmingdrmer mentioned this pull request Mar 9, 2024
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.

Refactor snapshot API Consider refactoring Snapshot type bound.
2 participants