gc_worker: use async_snapshot instead of raw API in GC by SpadeA-Tang · Pull Request #13322 · tikv/tikv

SpadeA-Tang · 2022-08-22T06:27:12Z

What is changed and how it works?

Issue Number: Close #13319

What's Changed:

In the past, we have some raw APIs in Engine trait to avoid breaking hinerrnate regions when doing GC works: snapshot_on_kv_engine and modify_on_kv_engine.

Now, we have supported stale read and so we can use stale read + start_ts 0 to acquire the snapshot which is implemented in this PR. In addition, this PR provider a way to fetch the inner kv engine from the snapshot.

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

Unit test
Integration test

Side effects

Performance regression
- Consumes more CPU
- Consumes more MEM
Breaking backward compatibility

Release note

None

Signed-off-by: SpadeA-Tang <[email protected]>

ti-chi-bot · 2022-08-22T06:27:13Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

BusyJay
tonyxuqqi

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: SpadeA-Tang <[email protected]>

SpadeA-Tang · 2022-08-23T11:47:26Z

/test

Signed-off-by: SpadeA-Tang <[email protected]>

BusyJay · 2022-09-01T11:01:23Z

components/server/src/server.rs

            self.pd_client.feature_gate().clone(),
+            Arc::new(self.region_info_accessor.clone()),
        );
-        gc_worker


/cc @MyonKeminta PTAL

Starting of the gc_worker is delayed to init_servers function, just before start_auto_gc. The purpose seems to be getting the store id when starting the gc_worker. I think delaying the starting of gc_worker doesn't cause any severe problem if test passes, but I would suggest wrapping the three "start" functions into a single start_gc_worker function.

BusyJay · 2022-09-02T06:26:24Z

src/server/gc_worker/gc_worker.rs

                .debug_struct("Gc")
-                .field("start_key", &log_wrappers::Value::key(start_key))
-                .field("end_key", &log_wrappers::Value::key(end_key))
+                .field(


Why not .field("region", region)?

To be consistent with before. Other infomation in Region may not be needed.

Using region is OK.

BusyJay · 2022-09-02T07:36:13Z

src/server/gc_worker/gc_worker.rs

+// all regions of thoes keys.
+// We return an iterator which yields items of `Key` and the region taht the key
+// is located.
 fn get_keys_in_regions(


This seems a bit over complicated. Why not just use 2 for loops?

for region in regions { let keys = get_keys_in_region(&keys, region); // process keys }

BusyJay · 2022-09-02T08:21:03Z

tests/failpoints/cases/test_stale_read.rs

 }
+
+#[test]
+fn test_stale_read_with_ts0() {


If no failpoint is used, then it should be put in integration directory.

BusyJay · 2022-09-02T08:23:32Z

tests/integrations/storage/test_storage.rs

+
+    let store_id = 1;
+    let mut region = metapb::Region::default();
+    region.set_peers(RepeatedField::from_vec(vec![metapb::Peer {


You mean region.mut_peers().push(new_peer(store_id, 0))?

There are still many places using RepeatedField.

BusyJay · 2022-09-02T08:25:24Z

components/test_storage/Cargo.toml

 futures = "0.3"
 kvproto = { git = "https://github.com/pingcap/kvproto.git" }
 pd_client = { path = "../pd_client", default-features = false }
+protobuf = { version = "2.8", features = ["bytes"] }


This dependency should not be added.

Signed-off-by: SpadeA-Tang <[email protected]>

BusyJay · 2022-09-02T10:32:27Z

components/test_storage/src/assert_storage.rs

        let safe_point = safe_point.into();
        for _ in 0..3 {
-            let ret = self.store.gc(self.ctx.clone(), safe_point);
+            let ret = self.store.gc(region, self.ctx.clone(), safe_point);


Why not just region.clone()? So you don't need to change the other function's definition.

The region may be changed.

BusyJay · 2022-09-02T10:39:38Z

src/server/gc_worker/gc_manager.rs

+        if region.is_none() {
+            return Ok(None);
        };
+        let region = region.unwrap();


Suggested change

if region.is_none() {

return Ok(None);

};

let region = region.unwrap();

let Some(region) = region else { return Ok(None) };

BusyJay · 2022-09-02T10:53:34Z

src/server/gc_worker/gc_worker.rs

                .debug_struct("Gc")
-                .field("start_key", &log_wrappers::Value::key(start_key))
-                .field("end_key", &log_wrappers::Value::key(end_key))
+                .field(


Using region is OK.

BusyJay · 2022-09-02T10:54:38Z

src/server/gc_worker/gc_worker.rs

@@ -247,40 +247,129 @@ struct KeysInRegions<R: Iterator<Item = Region>> {
 }

 impl<R: Iterator<Item = Region>> Iterator for KeysInRegions<R> {


This can be removed.

BusyJay · 2022-09-02T10:58:15Z

src/server/gc_worker/gc_worker.rs

+fn get_regions_for_gc(
+    store_id: u64,
+    keys: &Vec<Key>,
+    region_or_provider: Either<Region, Arc<dyn RegionInfoProvider>>,


It can just take Arc<dyn RegionInfoProvider>. If it's Either::Left, there is no need to call this method.

Signed-off-by: SpadeA-Tang <[email protected]>

BusyJay · 2022-09-02T12:42:07Z

src/server/gc_worker/gc_worker.rs

+// Return regions that keys are related to.
+fn get_regions_for_gc(
+    store_id: u64,
+    keys: &Vec<Key>,


Should be &[Key].

BusyJay · 2022-09-02T12:42:39Z

src/server/gc_worker/gc_worker.rs

+        Ok(regions)
+    } else {
+        // We only have one key.
+        let key = keys.first().unwrap().as_encoded();


Should be just keys[0].as_encoded().

BusyJay · 2022-09-02T12:45:05Z

src/server/gc_worker/gc_worker.rs

+        let end = keys.last().unwrap().as_encoded();
+        let regions = box_try!(region_provider.get_regions_in_range(start, end))
+            .into_iter()
+            .filter(move |r| find_peer(r, store_id).is_some())


move is unnecessary.

BusyJay · 2022-09-02T12:49:37Z

src/server/gc_worker/gc_worker.rs

+
+    match box_try!(rx.recv()) {
+        Some(region) => Ok(region),
+        None => unreachable!(),


Why is it unreachable? What if there is no replicas on the node?

BusyJay · 2022-09-02T12:59:18Z

src/server/gc_worker/gc_worker.rs

-            MvccReader::new(snapshot, Some(ScanMode::Forward), false)
+        let (mut handled_keys, mut wasted_keys) = (0, 0);
+        let mut regions = match region_or_provider {
+            Either::Left(region) => vec![region].into_iter().peekable(),


You don't need that. Vec<Region>[0] is the same as Vec.into_iter().peekable().peek().

BusyJay · 2022-09-02T13:00:24Z

src/server/gc_worker/gc_worker.rs

-        let (mut handled_keys, mut wasted_keys) = (0, 0);
+        // First item is fetched to initialize the reader and kv_engine
+        let region = regions.peek();
+        if region.is_none() {


regions.is_empty() is more straightforward.

BusyJay · 2022-09-02T13:04:06Z

src/server/gc_worker/gc_worker.rs

-                gc_info.is_completed = true;
+        let mut keys = keys.into_iter().peekable();
+        for region in regions {
+            if !first_iteration {


Why not

for region in regions { let (reader, kv_engine) = self.create_reader()?; let txn = Self::new_txn(); xxxx; Self::flush_txn(); }

When one tablet per region, you can't flush transaction across tablet.

But it can also have single-rocksdb setting.

Interesting. In the future, it may need to send the modifies to apply worker by region instead.

Signed-off-by: SpadeA-Tang <[email protected]>

BusyJay

Rest LGTM

BusyJay · 2022-09-02T15:00:02Z

src/server/gc_worker/gc_worker.rs

        let mut gc_info = GcInfo::default();
        let mut keys = keys.into_iter().peekable();
-        for region in regions {
+        for region in regions.into_iter() {


Why? Vec can be used directly.

BusyJay · 2022-09-05T02:02:43Z

src/server/gc_worker/gc_worker.rs

+        let key = keys.peek();
+        if key.is_none() {
+            break;
+        }
+        let key = key.unwrap().as_encoded().as_slice();


Suggested change

let key = keys.peek();

if key.is_none() {

break;

}

let key = key.unwrap().as_encoded().as_slice();

let Some(key) = keys.peek() else { break };

BusyJay · 2022-09-05T03:36:44Z

components/engine_rocks/src/snapshot.rs

        }
    }
+
+    pub fn db(&self) -> Arc<DB> {


Not necessary anymore, people can use as_inner.

BusyJay · 2022-09-05T03:53:20Z

src/server/gc_worker/gc_worker.rs

-        let (locks, _) = reader
-            .scan_locks(Some(start_key), None, |l| l.ts <= max_ts, limit)
-            .map_err(TxnError::from_mvcc)?;
+        let regions = box_try!(regions_provider.get_regions_in_range(start_key.as_encoded(), &[]))


We need to evaluate the performance impact of changing from one seek to multiple seek.

As Green GC is going to be deprecated, I think we don't need to concern about it too much.
Maybe it is the time to deprecate it now, since only scanning regions found in region_info_provider may possibly affect the scanning's correctness.

BusyJay · 2022-09-05T03:56:02Z

src/server/gc_worker/gc_worker.rs

+            let snap = self.get_snapshot(self.store_id, &region)?;
+            let mut reader = MvccReader::new(snap, Some(ScanMode::Forward), false);
+            let (locks_this_region, _) = reader
+                .scan_locks(Some(&start_key), None, |l| l.ts <= max_ts, limit)


/cc @lhy1024 Will it impact the statistics? As there will be many scan ops and end key is always None.

Scan is also limited by region boundries of snapshot.

If this pr only affects gc, it has no effect on hotspot statistics, because pd does not process gc-related statistics for the time being

Signed-off-by: SpadeA-Tang <[email protected]>

MyonKeminta · 2022-09-05T07:07:17Z

/merge

ti-chi-bot · 2022-09-05T07:07:18Z

@MyonKeminta: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-09-05T07:07:21Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: bc9deea

ti-chi-bot · 2022-09-05T07:07:33Z

@SpadeA-Tang: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

tabokie · 2022-09-08T14:33:24Z

src/server/gc_worker/gc_worker.rs


    pub fn start_auto_gc<S: GcSafePointProvider, R: RegionInfoProvider + Clone + 'static>(
        &self,
+        kv_engine: &E::Local,


@SpadeA-Tang Could you help explain a bit why this is necessary?

Okay... I have remembered why this change exists. I tried to remove the kv_engine() from Engine trait in some point of time but I gave up this way later without changeing it back. I will propose another PR for reverting it.

ref #13319, ref #13322 Signed-off-by: SpadeA-Tang <[email protected]> Co-authored-by: Xinye Tao <[email protected]>

SpadeA-Tang added 5 commits August 20, 2022 12:10

add inner_engine for Snapshot

4f91827

Signed-off-by: SpadeA-Tang <[email protected]>

save point

5ac509a

Signed-off-by: SpadeA-Tang <[email protected]>

add inner_engine for Snapshot and cargo c pass

e222edf

Signed-off-by: SpadeA-Tang <[email protected]>

make clippy for last commit

89676c0

Signed-off-by: SpadeA-Tang <[email protected]>

change GcTask::Gc for transmitting region information

41ebea4

Signed-off-by: SpadeA-Tang <[email protected]>

SpadeA-Tang added 5 commits August 22, 2022 16:38

modify get_keys_in_regions

5fc96f1

Signed-off-by: SpadeA-Tang <[email protected]>

make clippy pass

520ed60

Signed-off-by: SpadeA-Tang <[email protected]>

format

9dd8ad7

Signed-off-by: SpadeA-Tang <[email protected]>

add set_range for MvccReader

5226e70

Signed-off-by: SpadeA-Tang <[email protected]>

todo: test_gc_bypass_raft

104feb5

Signed-off-by: SpadeA-Tang <[email protected]>

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 22, 2022

SpadeA-Tang added 2 commits August 23, 2022 11:05

modify async_snapshot to use stale read

e5175be

Signed-off-by: SpadeA-Tang <[email protected]>

tackle tests

3af1d0a

Signed-off-by: SpadeA-Tang <[email protected]>

SpadeA-Tang added 5 commits August 24, 2022 10:34

amend range key

718878c

Signed-off-by: SpadeA-Tang <[email protected]>

tackle test

3e7b6e7

Signed-off-by: SpadeA-Tang <[email protected]>

update

8afc95f

Signed-off-by: SpadeA-Tang <[email protected]>

update

af8a5f9

Signed-off-by: SpadeA-Tang <[email protected]>

remove snapshot_on_kv_engine and modify_on_kv_engine

16aba6b

Signed-off-by: SpadeA-Tang <[email protected]>

SpadeA-Tang changed the title ~~WIP: use async_snapshot instead of raw API in GC~~ gc_worker: use async_snapshot instead of raw API in GC Aug 24, 2022

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 24, 2022

resolve merge conflict

292eb0b

Signed-off-by: SpadeA-Tang <[email protected]>

Merge branch 'master' into async_snapshot

60df411

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2022

BusyJay reviewed Sep 2, 2022

View reviewed changes

SpadeA-Tang added 2 commits September 2, 2022 17:15

update

23abd4f

Signed-off-by: SpadeA-Tang <[email protected]>

update

d837bb0

Signed-off-by: SpadeA-Tang <[email protected]>

BusyJay reviewed Sep 2, 2022

View reviewed changes

update

dfd55fa

Signed-off-by: SpadeA-Tang <[email protected]>

BusyJay reviewed Sep 2, 2022

View reviewed changes

SpadeA-Tang and others added 3 commits September 2, 2022 21:52

update

bcf7df4

Signed-off-by: SpadeA-Tang <[email protected]>

Merge branch 'master' into async_snapshot

46abc26

update

4e12f77

Signed-off-by: SpadeA-Tang <[email protected]>

BusyJay reviewed Sep 5, 2022

View reviewed changes

update

bc9deea

Signed-off-by: SpadeA-Tang <[email protected]>

BusyJay approved these changes Sep 5, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 5, 2022

MyonKeminta approved these changes Sep 5, 2022

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 5, 2022

Merge branch 'master' into async_snapshot

0bf2ac9

ti-chi-bot merged commit 7d36f34 into tikv:master Sep 5, 2022

ti-chi-bot added this to the Pool milestone Sep 5, 2022

tabokie reviewed Sep 8, 2022

View reviewed changes

This was referenced Sep 9, 2022

gc_worker: make background gc compatible with multi-rocks db #13402

Merged

gc_worker: revert some draft changes in PR#13322 #13471

Merged

ti-chi-bot pushed a commit that referenced this pull request Sep 16, 2022

gc_worker: revert some draft changes in PR#13322 (#13471)

2a67c47

ref #13319, ref #13322 Signed-off-by: SpadeA-Tang <[email protected]> Co-authored-by: Xinye Tao <[email protected]>

		@@ -247,40 +247,129 @@ struct KeysInRegions<R: Iterator<Item = Region>> {
		}

		impl<R: Iterator<Item = Region>> Iterator for KeysInRegions<R> {

Conversation

SpadeA-Tang commented Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

ti-chi-bot commented Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SpadeA-Tang commented Aug 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BusyJay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SpadeA-Tang commented Aug 22, 2022 •

edited

Loading

ti-chi-bot commented Aug 22, 2022 •

edited

Loading