Implement IngestSST API #2712

huachaohuang · 2018-01-23T16:20:14Z

An uploaded SST file can be ingested to a region by sending an IngestSST request. The client must guarantee that the SST file has been uploaded to each peer of the region before issuing the IngestSST request. Upon ingestion, the metadata of the SST file will be replicated to the raft group first. Then before the IngestSST command can be applied, the range and the epoch of the metadata must be checked, and the length and checksum of the coresponding SST file must match the metadata too. Finally, the SST file will be deleted after it has been applied.

The coresponding kvproto PR: pingcap/kvproto#224

overvenus · 2018-01-25T12:40:12Z

src/raftstore/store/store.rs

+            if let Some(region) = self.region_peers.get_mut(&sst.get_region_id()) {
+                region.size_diff_hint += sst.get_length();
+            }
+            let _ = self.importer.delete(sst);


Please error!() if it meets an error.

Errors are already logged in the importer.

Does it block the raftstore? If yes, it may need to move to another place.

I'm planning to add an import worker for the related jobs.

overvenus · 2018-01-25T12:48:33Z

src/raftstore/store/worker/apply.rs

+        || epoch.get_version() != region_epoch.get_version()
+    {
+        return Err(box_err!(
+            "region epoch does not match: {:?} != {:?}",


How about Error::StaleEpoch(_)?

overvenus · 2018-01-25T12:49:35Z

src/raftstore/store/worker/apply.rs

+    let region_id = sst.get_region_id();
+    if region_id != region.get_id() {
+        return Err(box_err!(
+            "region id does not match: {} != {}",


How about Error::RegionNotFound(_) ?

overvenus · 2018-01-25T13:14:03Z

src/storage/engine/mod.rs

@@ -71,6 +72,11 @@ pub enum Modify {

 pub trait Engine: Send + Debug {
    fn async_write(&self, ctx: &Context, batch: Vec<Modify>, callback: Callback<()>) -> Result<()>;
+
+    fn async_ingest(&self, _: &Context, _: SSTMeta, _: Callback<()>) -> Result<()> {


I don't think it is proper to define the async_ingest in the Engine.
I suggest constructing a RaftCmd in the ImportSSTService and send it to the raftstore.

huachaohuang · 2018-02-12T02:19:10Z

@zhangjinpeng1987 @overvenus PTAL

huachaohuang · 2018-02-26T03:08:13Z

PTAL @overvenus @zhangjinpeng1987

zhangjinpeng87 · 2018-02-26T08:09:38Z

src/import/sst_importer.rs

+    };
+
+    // A valid file name should be in the format:
+    // "{uuid}_{region_id}_{region_poch.conf_ver}_{region_epoch.version}.sst"


s/region_poch/region_epoch

zhangjinpeng87 · 2018-02-27T02:19:23Z

src/raftstore/store/worker/apply.rs

+            let _ = ctx.importer.delete(sst);
+            return Err(e);
+        }
+


It's better to add some metrics to indicate how many files we have ingested.

We can know that from the GRPC and RocksDB metrics.

zhangjinpeng87 · 2018-02-27T02:30:30Z

src/raftstore/store/worker/apply.rs

+            return Err(e);
+        }
+
+        ctx.importer.ingest(sst, &self.engine).unwrap_or_else(|e| {


Seems we don't verify checksum for this sst file?

The checksum is verified inside importer.ingest().

zhangjinpeng87 · 2018-02-27T02:40:46Z

src/raftstore/store/worker/cleanup_sst.rs

+    }
+
+    fn handle_delete_sst(&self, sst: SSTMeta) {
+        let _ = self.importer.delete(&sst);


Any mechanism to clean ssts when delete failed?

Yes, there is a mechanism to check and clean ssts periodically in the following PR.

zhangjinpeng87 · 2018-02-27T09:00:37Z

LGTM

overvenus · 2018-02-27T10:10:06Z

src/import/service.rs


 use super::Error;

+pub fn make_cb<T: Debug + Send + 'static>() -> (Box<FnBox(T) + Send>, oneshot::Receiver<T>) {


It is the same as service::kv::make_callnack.

Yes, but I don't want to rely on a private function there, and it is just a simple function, so I make it self-contained here.

Now we have a helper function.

https://github.com/pingcap/tikv/blob/0a991685770ed84f1e3db9b0f9870e5a77bf2778/src/util/future.rs#L19

overvenus · 2018-02-27T10:13:40Z

src/import/errors.rs

@@ -52,6 +54,16 @@ quick_error! {
            from()
            display("RocksDB {}", msg)
        }
+        RaftStore(err: RaftStoreError) {
+            from()
+                cause(err)


Please keep Indents.

overvenus · 2018-02-27T11:56:31Z

src/raftstore/store/worker/cleanup_sst.rs

+use util::worker::Runnable;
+
+pub enum Task {
+    DeleteSST { sst: SSTMeta },


Can it be handled by the apply worker?

We need to delete the SST after it is successfully applied, so according to the current applying flow, it is more intuitive to handle it when we receive the ExecResult.

overvenus · 2018-02-27T12:00:03Z

tests/import/sst_service.rs

@@ -43,22 +45,24 @@ fn new_cluster() -> (Cluster<ServerCluster>, Context) {
    (cluster, ctx)
 }

-fn new_cluster_and_import_client() -> (Cluster<ServerCluster>, ImportSstClient) {
+fn new_cluster_and_tikv_import_client(
+) -> (Cluster<ServerCluster>, Context, TikvClient, ImportSstClient) {


Weird format.

It is formatted by rustfmt :)

huachaohuang · 2018-03-07T07:01:05Z

@overvenus PTAL

overvenus

Rest LGTM

zhangjinpeng87 · 2018-03-14T10:52:17Z

Cargo.lock

@@ -422,7 +422,7 @@ dependencies = [
 [[package]]
 name = "kvproto"
 version = "0.0.1"
-source = "git+https://github.com/pingcap/kvproto.git#123051ec2973aa59a4d6d5b2c5da00071c0ddd24"
+source = "git+https://github.com/huachaohuang/kvproto.git?branch=ingest-sst#e194fb28415314427ad6a2497b65315db92745ee"


Why it is still in your branch?

Client must guarantee that the SST file has been uploaded to the servers where the region's peers locate in, before issuing an ingest request to the region's leader. The SST metadata provided in the ingest request will first be replicated through Raft. Then when the log entry is applied, the coresponding uploaded SST file will be ingested to the local engine. To make sure that the region's range and peers are not changed on during this process, the region's epoch will be checked before ingestion.

huachaohuang · 2018-03-15T01:36:52Z

/run-integration-tests

zhangjinpeng87

LGTM

* Implement ImportSST::Ingest API Client must guarantee that the SST file has been uploaded to the servers where the region's peers locate in, before issuing an ingest request to the region's leader. The SST metadata provided in the ingest request will first be replicated through Raft. Then when the log entry is applied, the coresponding uploaded SST file will be ingested to the local engine. To make sure that the region's range and peers are not changed on during this process, the region's epoch will be checked before ingestion.

huachaohuang requested review from overvenus and zhangjinpeng87 January 23, 2018 16:20

overvenus reviewed Jan 25, 2018

View reviewed changes

zhangjinpeng87 reviewed Feb 26, 2018

View reviewed changes

zhangjinpeng87 reviewed Feb 27, 2018

View reviewed changes

overvenus reviewed Feb 27, 2018

View reviewed changes

huachaohuang mentioned this pull request Mar 1, 2018

Revert "Add IngestSST Command (#224)" pingcap/kvproto#233

Merged

overvenus reviewed Mar 13, 2018

View reviewed changes

overvenus previously approved these changes Mar 14, 2018

View reviewed changes

zhangjinpeng87 reviewed Mar 14, 2018

View reviewed changes

huachaohuang added 2 commits March 15, 2018 01:15

Update kvproto

f91b686

huachaohuang dismissed overvenus’s stale review via f91b686 March 14, 2018 17:38

zhangjinpeng87 approved these changes Mar 15, 2018

View reviewed changes

huachaohuang merged commit d94db7b into tikv:master Mar 15, 2018

huachaohuang deleted the ingest-sst branch April 8, 2018 07:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement IngestSST API #2712

Implement IngestSST API #2712

huachaohuang commented Jan 23, 2018 •

edited

overvenus Jan 25, 2018

huachaohuang Jan 26, 2018

overvenus Jan 30, 2018

huachaohuang Feb 9, 2018

overvenus Jan 25, 2018

overvenus Jan 25, 2018

overvenus Jan 25, 2018

huachaohuang Jan 26, 2018

huachaohuang commented Feb 12, 2018

huachaohuang commented Feb 26, 2018

zhangjinpeng87 Feb 26, 2018

zhangjinpeng87 Feb 27, 2018

huachaohuang Feb 27, 2018

zhangjinpeng87 Feb 27, 2018

huachaohuang Feb 27, 2018

zhangjinpeng87 Feb 27, 2018

huachaohuang Feb 27, 2018

zhangjinpeng87 commented Feb 27, 2018

overvenus Feb 27, 2018

huachaohuang Feb 27, 2018

overvenus Mar 13, 2018

overvenus Feb 27, 2018

overvenus Feb 27, 2018

huachaohuang Feb 27, 2018

overvenus Feb 27, 2018

huachaohuang Feb 27, 2018

huachaohuang commented Mar 7, 2018

overvenus left a comment

zhangjinpeng87 Mar 14, 2018

huachaohuang commented Mar 15, 2018

zhangjinpeng87 left a comment


		use super::Error;

		pub fn make_cb<T: Debug + Send + 'static>() -> (Box<FnBox(T) + Send>, oneshot::Receiver<T>) {

Implement IngestSST API #2712

Implement IngestSST API #2712

Conversation

huachaohuang commented Jan 23, 2018 • edited

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

huachaohuang commented Feb 12, 2018

huachaohuang commented Feb 26, 2018

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

zhangjinpeng87 commented Feb 27, 2018

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

huachaohuang commented Mar 7, 2018

overvenus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huachaohuang commented Mar 15, 2018

zhangjinpeng87 left a comment

Choose a reason for hiding this comment

huachaohuang commented Jan 23, 2018 •

edited