add sst file snapshot impl #1618

hhkbp2 · 2017-02-15T01:37:51Z

Hi,

This PR tries to add impl for sst file snapshot. It's split from PR #1561

siddontang · 2017-02-16T08:34:34Z

Cargo.toml

@@ -62,6 +62,7 @@ signal = "0.2"

 [dependencies.rocksdb]
 git = "https://github.com/pingcap/rust-rocksdb.git"
+branch = "hhkbp2/sst-file-snapshot"


can use master now

siddontang · 2017-02-16T08:38:05Z

src/raftstore/store/snap.rs

+        let mut buf = vec![0; DIGEST_BUFFER_SIZE];
+        loop {
+            let n = try!(f.read(&mut buf[..]));
+            if n == 0 {


digest.write(&but[..n]) if n < buf.len() { return sum }

siddontang · 2017-02-16T08:44:01Z

src/raftstore/store/snap.rs

+        }
+    }
+
+    fn parse_cf_size_checksums(kvs: &[KeyValue]) -> RaftStoreResult<Vec<(String, u64, u32)>> {


I prefer define a struct to contain the meta and use json to do the codec directly, no need to encode/decode manually.

Btw, we can also define a protobuf type too. But json is simple here.

siddontang · 2017-02-16T08:49:39Z

src/raftstore/store/snap.rs

+                try!(fs::create_dir_all(dir_path.as_path()));
+            }
+            let prefix = format!("{}_{}", SNAP_GEN_PREFIX, key);
+            let display_path = Snap::get_display_path(&dir_path, &prefix);


using a unique folder to save all CF SST files and meta may be better? like /dir/gen_{key}/

so we can rename, delete the folder directly later.

It's commonly seen that a single directory is used for raw data files, e.g. RocksDB use a single db dir to store sst files of all LSM levels.

siddontang · 2017-02-16T08:54:24Z

src/raftstore/store/snap.rs

+                if !need_to_pack(cf) {
+                    continue;
+                }
+                try!(self.next_file(cf.to_owned()));


seem that it is not for next file, but to get the correct writer index with the CF.

siddontang · 2017-02-16T08:55:56Z

src/raftstore/store/snap.rs

+            if buf.len() == 0 {
+                return Ok(0);
+            }
+            while self.cf_index < self.cf_files.len() {


why not using enumerate directly?

It needs to record the current index to self.cf_index.

siddontang · 2017-02-16T08:59:54Z

src/raftstore/store/snap.rs

+            let mut f = self.meta_file.file.as_mut().unwrap();
+            try!(f.write_all(&v[..]));
+            try!(f.flush());
+            try!(fs::rename(&self.meta_file.tmp_path, &self.meta_file.path));


the file is still opened, can it be renamed?

siddontang · 2017-02-16T09:16:31Z

PTAL @BusyJay @zhangjinpeng1987

hhkbp2 · 2017-02-22T02:59:39Z

PTAL @siddontang @BusyJay @zhangjinpeng1987

BusyJay · 2017-03-24T12:10:21Z

src/raftstore/store/snap.rs

+                                                      meta.get_size(),
+                                                      meta.get_checksum()));
+                }
+                cf_file.size = meta.get_size();


Is it a bug that meta exists but the cf file doesn't?

What do you mean exactly "is it a bug"?
The data corruption could cause this exception: the cf files are deleted/gone, but the meta file remains.

Then it should return an error.

The checking is added in load_snapshot_meta().

BusyJay · 2017-03-24T12:22:02Z

src/raftstore/store/snap.rs

+                }
+                let meta = found.unwrap();
+                if file_exists(&cf_file.path) {
+                    try!(check_file_size_and_checksum(&cf_file.path,


If it's about to generate the snapshot, when either file size or checksum doesn't match, the file should be deleted; otherwise should panic.

address comment. The panic should be raised properly by upper level code.

BusyJay · 2017-03-24T12:25:46Z

src/raftstore/store/snap.rs

+            }
+            let file = try!(OpenOptions::new()
+                .write(true)
+                .create_new(true)


This may fail if meta file exists.

The meta file is written at last. If the meta file exists, this snapshot exists(normally, if there is no data corruption). It checks exists() at the begin of this function. So if the meta file exists, the control flow will not get to this create_new().

What if the meta file exists but some cf files are missing?

Then it must be through some kind of data corruption, then it return an error here.

BusyJay · 2017-03-24T12:28:09Z

src/raftstore/store/snap.rs

+            // set snapshot meta data
+            let total_size = try!(self.total_size());
+            snap_data.set_file_size(total_size);
+            snap_data.set_version(SNAPSHOT_VERSION);


It seems v2 or v1 share the same SNAPSHOT_VERSION.

My mistake, they don't.

BusyJay · 2017-03-24T12:31:14Z

src/raftstore/store/snap.rs

+                    stat: &mut SnapshotStatistics)
+                    -> RaftStoreResult<()> {
+            if self.exists() {
+                match self.validate() {


The file is checked twice when it exists already. First time is in init_for_building, second time is here.

What's the point here? Are you suggesting merging two piece of code together?

What I mean is the file only needs to be checked once. Either checks can be removed.

They are two pieces of code which are called on initialization and building. And the exists() checks the existence for different purpose. It's doable to add some flag var like exists, but it's simpler to just call a simple function exists().

BusyJay · 2017-03-24T12:41:55Z

src/raftstore/store/snap.rs

+
+    pub struct Snap {
+        display_path: String,
+        cf_files: Vec<CfFile>,


Seems using HashMap is more readable than a Vec and an index.

It's good to use a simple vector and index combination. The vector specifies the cf sequence, the index could be set to upper bound to indicate that there is no more capacity to read/to write. And it's simple and fast.

BusyJay · 2017-03-24T12:46:01Z

src/raftstore/store/snap.rs

+                        return Err(io::Error::new(ErrorKind::Other, e));
+                    }
+                }
+                let size = try!(get_file_size(&cf_file.tmp_path));


Is it possible that the size doesn't match the actual file when the file handle is dropped without flush?

This function only runs when a snapshot is built, so the cf file size is first initialized as snapshot meta. And the file is always being flushed when closed.
I don't get it by "the size doesn't match the actual file"? Could you elaborate on this?

BusyJay · 2017-03-24T12:50:18Z

src/raftstore/store/snap.rs

+                    cf_file.checksum = try!(calc_crc32(&cf_file.path));
+                } else {
+                    // Clean up the `tmp_path` if this cf file is empty.
+                    delete_file_if_exist(&cf_file.tmp_path);


The clean up job should be implemented in Drop trait in case there is any missing condition.

Don't know what you mean by the missing condition. Drop::drop takes care of the situation where the snapshot file is written partly on error.

I thought Drop was not implemented when I saw the temp file was deleted explicitly here.

BusyJay · 2017-03-24T12:56:02Z

src/raftstore/store/snap.rs

+            fs::metadata(&self.meta_file.path)
+        }
+
+        fn total_size(&self) -> io::Result<u64> {


Why not include the size of meta file?

total_size returns the total size appended after the snapshot meta.

BusyJay · 2017-03-24T12:59:17Z

src/raftstore/store/snap.rs

+            snap_data.set_meta(self.meta_file.meta.clone());
+
+            // add size
+            let mut size_track = self.size_track.wl();


The size should be updated once the snapshot file is saved, otherwise if there are several failure, there will be a big gap between the size_track and the actual snap directory size.

It's updated after the snapshot file is saved, actually. It will not get to any error after the meta file is done saving.

What I mean is it should be updated at https://github.com/pingcap/tikv/pull/1618/files#diff-d73c1547780d463f5ca96c85a279da7cR1074.

BusyJay · 2017-03-24T13:31:27Z

src/raftstore/store/snap.rs

 use util::transport::SendCh;
 use util::{HandyRwLock, HashMap};

 use super::engine::Snapshot as DbSnapshot;
 use super::peer_storage::JOB_STATUS_CANCELLING;

+// Data in CF_RAFT should be excluded for a snapshot.
+const SNAPSHOT_CFS: &'static [CfName] = &[CF_DEFAULT, CF_LOCK, CF_WRITE];


This can be used in all the place related to snapshot too, like region.rs.

BusyJay · 2017-03-24T14:20:41Z

If there are snapshot files that have no correspond meta file, they won't be deleted by gc.

BusyJay · 2017-03-24T14:22:51Z

src/raftstore/store/snap.rs

+                                                   snapshot_meta: SnapshotMeta,
+                                                   size_track: Arc<RwLock<u64>>)
+                                                   -> RaftStoreResult<Snap> {
+            let cfs = try!(check_snapshot_meta(&snapshot_meta));


The check is redundant.

Why it's redundant? This check_snapshot_meta plays its role to check the validity of the snapshot meta.

set_snapshot_meta should do the job properly.

set_snapshot_meta checks whether the size and checksum is correct when snapshot file exists. check_snapshot_meta checks whether the received snapshot meta has valid cf name and number. They are different.

I don't think we need two function to do the check. When setting snapshot meta, the size, checksum and cfs all should be as expected.

hhkbp2 · 2017-03-25T11:26:38Z

If there are snapshot files that have no correspond meta file, they won't be deleted by gc.

It will be drop during gc procedure.
@BusyJay

BusyJay · 2017-03-25T13:44:09Z

No, for sending, it will report error at https://github.com/pingcap/tikv/blob/hhkbp2/sst-file-snapshot-v2/src/raftstore/store/snap.rs#L887; for receiving, it will be skipped at https://github.com/pingcap/tikv/blob/hhkbp2/sst-file-snapshot-v2/src/raftstore/store/snap.rs#L1823.

hhkbp2 · 2017-03-26T08:00:44Z

PTAL

zhangjinpeng87 · 2017-03-26T12:53:38Z

src/raftstore/store/snap.rs

+        }
+    }
+
+    fn get_snapshot_meta(cf_files: &[CfFile]) -> RaftStoreResult<SnapshotMeta> {


s/get_snapshot_meta/gen_snapshot_meta

zhangjinpeng87 · 2017-03-27T06:16:49Z

LGTM

hhkbp2 · 2017-03-27T06:26:25Z

PTAL @BusyJay

BusyJay · 2017-03-27T07:07:02Z

src/raftstore/store/snap.rs

+            // load snapshot meta if meta_file exists
+            if file_exists(&s.meta_file.path) {
+                if let Err(e) = s.load_snapshot_meta() {
+                    if !to_build {


You can load the meta lazily instead of passing a to_build explicitly.

The meta is needed in the most common interface exists(). Snap is usually used as a short-time living object in the context. It's not shared or reused. It would not benefit to delay the meta loading, but just make the code complicated.

BusyJay · 2017-03-27T07:57:02Z

src/util/file.rs

+}
+
+pub fn delete_file_if_exist(file: &PathBuf) {
+    if !file_exists(file) {


The file can be removed directly and then omit the not found error. So there will be only once sys call.

BusyJay · 2017-03-27T08:01:58Z

src/raftstore/store/snap.rs

+            try!(self.meta_file.meta.write_to_vec(&mut v));
+            try!(self.meta_file.file.take().unwrap().write_all(&v[..]));
+            try!(fs::rename(&self.meta_file.tmp_path, &self.meta_file.path));
+            let mut size_track = self.size_track.wl();


Should be updated at L1235.

BusyJay · 2017-03-27T08:21:59Z

src/raftstore/store/snap.rs

+            Ok(snapshot_meta)
+        }
+
+        fn set_snapshot_meta(&mut self, snapshot_meta: SnapshotMeta) -> RaftStoreResult<()> {


I think it should be checked if the cfs in snapshot_meta is the same as SNAPSHOT_CFS.

hhkbp2 · 2017-03-27T11:17:23Z

Ping @BusyJay

BusyJay · 2017-03-27T11:23:12Z

src/raftstore/store/snap.rs

                let found = snapshot_meta.get_cf_files().iter().find(|x| x.get_cf() == cf_file.cf);
                if found.is_none() {
-                    return Err(box_err!("cf {} doesn't exist in snapshot meta", cf_file.cf));
+


Seems L977~L981 can be removed.

My bad. Forget to clean up the old code.

BusyJay

rest LGTM

BusyJay · 2017-03-27T11:45:33Z

LGTM

hhkbp2 added 2 commits February 14, 2017 16:54

raftstore: add sst file snapshot impl

a2fd4cd

add impl

a4688fe

hhkbp2 mentioned this pull request Feb 15, 2017

use rocksdb sst file ingestion for snapshot #1561

Closed

siddontang reviewed Feb 16, 2017

View reviewed changes

hhkbp2 added 7 commits February 17, 2017 10:13

update rocksdb

632dc1b

Merge branch 'master' into hhkbp2/sst-file-snapshot-v2

dac92aa

add test cases

d8ed420

revise test case

4a4d846

add test case

76805f1

revise test case

125616c

add test case

e8badb3

hhkbp2 changed the title ~~[WIP] add sst file snapshot impl~~ add sst file snapshot impl Feb 21, 2017

Merge branch 'master' into hhkbp2/sst-file-snapshot-v2

30f9373

hhkbp2 added 9 commits February 23, 2017 10:56

fix typo

1921afc

add size checking

be80312

remove duplicated delete()

2269d98

add debug log for receiving cf files

c04a5dd

add max size checking for cf file

10245aa

fix write size bug

dee9bb7

Merge branch 'master' into hhkbp2/sst-file-snapshot-v2

fa2efe7

change cf file max size

ec43384

add test case

2a5f036

BusyJay reviewed Mar 24, 2017

View reviewed changes

hhkbp2 added 2 commits March 24, 2017 22:59

address comment

54793f1

address comments

49f04a8

hhkbp2 added 4 commits March 25, 2017 21:51

Merge branch 'master' into hhkbp2/sst-file-snapshot-v2

a9ab1e6

address comment

5ae6704

remove snapshot meta checking

e516512

address comment

d0a68e0

zhangjinpeng87 reviewed Mar 26, 2017

View reviewed changes

address comment

853f58f

BusyJay reviewed Mar 27, 2017

View reviewed changes

address comments

9458f22

BusyJay reviewed Mar 27, 2017

View reviewed changes

hhkbp2 added 2 commits March 27, 2017 19:39

remove obsolete code

45e6b84

Merge branch 'master' into hhkbp2/sst-file-snapshot-v2

187a27f

ngaut merged commit 9bbb312 into master Mar 27, 2017

ngaut deleted the hhkbp2/sst-file-snapshot-v2 branch March 27, 2017 12:15

sre-bot added the contribution Type: PR - From contributors label Dec 18, 2019

absolute8511 mentioned this pull request Jun 17, 2021

Improve the snapshot restore youzan/ZanRedisDB#92

Open

add sst file snapshot impl #1618

add sst file snapshot impl #1618

Conversation

hhkbp2 commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddontang commented Feb 16, 2017

hhkbp2 commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BusyJay commented Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhkbp2 commented Mar 25, 2017 • edited Loading

BusyJay commented Mar 25, 2017

hhkbp2 commented Mar 26, 2017

Choose a reason for hiding this comment

zhangjinpeng87 commented Mar 27, 2017

hhkbp2 commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhkbp2 commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BusyJay left a comment

Choose a reason for hiding this comment

BusyJay commented Mar 27, 2017

BusyJay commented Mar 24, 2017 •

edited

Loading

hhkbp2 commented Mar 25, 2017 •

edited

Loading