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

raft_serverpb: add the extra trait use_plain_file for SnapshotMeta #1260

Closed

Conversation

LykxSassinator
Copy link
Contributor

Description

This PR is a pre-requisite for TiKV #https://github.com/tikv/tikv/pull/17408/files, used for adding a new trait use_plain_file for SnapshotMeta. And this trait is introduced for marking that the relative snapshot can be flushed with plain file or sst files.

@ti-chi-bot ti-chi-bot bot requested review from JmPotato and rleungx August 29, 2024 03:02
Copy link

ti-chi-bot bot commented Aug 29, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign overvenus for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@@ -31,7 +30,6 @@ var _ status.Status
var _ = runtime.String
var _ = utilities.NewDoubleArray
var _ = descriptor.ForMessage
var _ = metadata.Join
Copy link
Member

Choose a reason for hiding this comment

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

Why is this file changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's cleaned by script automatically.

@@ -59,6 +59,9 @@ message SnapshotMeta {
// It should only be used for v2 to send snapshot to v1.
// See https://github.com/pingcap/tiflash/issues/7568
uint64 commit_index_hint = 7;
// Flag reflects whether the snapshot is persisted with plain files
// or sst files. `true` means using plain files.
bool use_plain_file = 8;
Copy link
Member

Choose a reason for hiding this comment

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

How about adding it to SnapshotCFFile? It's more flexible, eg. index regions have little data in the default CF but a large amount of data in the write CF.

Copy link
Contributor Author

@LykxSassinator LykxSassinator Sep 2, 2024

Choose a reason for hiding this comment

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

Reasonable but heavy on scanning. One concern is that we needs to manual scan to get the size of each cf, which will costs a lot on the IO and CPU when dumping the snapshot.
So, it's added into SnapshotMeta, and whether the region should use_plain_file == true is depends on the approximate size of the region is smaller than the given threshold.

@LykxSassinator
Copy link
Contributor Author

To ensure the forward compatibility of TiKV, this change can be replaced with another implementation, by doing apply check on the recv side of TiKV.
So, this change is not necessary and can be safely closed.

@LykxSassinator LykxSassinator deleted the add_extra_snap_meta branch September 10, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants