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.

Sign up for GitHub

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

Jump to bottom

gc: support multi-rocksdb in compaction filter #13228

Closed
wants to merge 55 commits into from
Closed

gc: support multi-rocksdb in compaction filter #13228

wants to merge 55 commits into from

Conversation

BornChanger
Copy link
Contributor

@BornChanger BornChanger commented Aug 4, 2022 •
edited

Signed-off-by: BornChanger dawn_catcher@126.com

What is changed and how it works?

Issue Number: Close #13131

What's Changed:

Associate proper region id, suffix id and optional engine factory to WriteCompactionFilterFactory and RawCompactionFilterFactory at cfs option creation time. At create_compaction_filter time, retrieve the proper rocksdb engine instead of db in the global GC_CONTEXT.

Related changes

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

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Release note 

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 4, 2022 •
edited

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • 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.

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

@ti-chi-bot ti-chi-bot added release-note contribution Type: PR - From contributors size/M labels Aug 4, 2022
@BornChanger
Copy link
Contributor Author

/run-test

tonyxuqqi
tonyxuqqi reviewed Aug 11, 2022
View reviewed changes
src/config.rs Outdated Show resolved Hide resolved
@BornChanger
Copy link
Contributor Author

/run-test

@BornChanger
Copy link
Contributor Author

/cc @tabokie

@BornChanger
Copy link
Contributor Author

cc @tabokie

@BornChanger BornChanger marked this pull request as draft August 18, 2022 08:49
@BornChanger BornChanger force-pushed the gc_multi_rocksdb branch 2 times, most recently from 0c21dab to b5c915c Compare August 18, 2022 14:45
@BornChanger BornChanger marked this pull request as ready for review August 30, 2022 03:55
@BornChanger BornChanger force-pushed the gc_multi_rocksdb branch 3 times, most recently from 974ebed to c4258d4 Compare August 30, 2022 04:50
@BornChanger
Copy link
Contributor Author

/run-clippy-linux-arm64

@BornChanger BornChanger requested review from tonyxuqqi and removed request for tabokie August 30, 2022 15:24
@BornChanger
Copy link
Contributor Author

/run-test retry=7

1 similar comment
@BornChanger
Copy link
Contributor Author

/run-test retry=7

tabokie
tabokie reviewed Oct 24, 2022
View reviewed changes
components/raftstore/src/coprocessor/region_info_accessor.rs Outdated Show resolved Hide resolved
components/server/src/server.rs Outdated Show resolved Hide resolved
components/engine_test/src/lib.rs Outdated Show resolved Hide resolved
tests/failpoints/cases/test_gc_worker.rs Outdated Show resolved Hide resolved
tests/failpoints/cases/test_gc_worker.rs Outdated
let ops = DbOptions::default();
let path = Builder::new()
.prefix("test_gc_keys_with_region_info_provider")
.tempdir()
Copy link
Member

Choose a reason for hiding this comment

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

All the usage of tempdir is incorrect. Because when the Tempdir object goes out of scope, the directory will be automatically deleted, and the engine data will be lost too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you said is right. I have corrected other occurrence for path construction except for prepare_gc(). At prepare_gc() invocation time, engine is ready (passed as one parameter), so the temp dir dropping is ok. It's just a skeleton for factory construction, while the factory won't really create any rocksdb later and is only for storing the registry of <<region_id, suffix_id>, rocksdb> in GC_Context.

tests/failpoints/cases/test_table_properties.rs Outdated
@@ -167,6 +168,7 @@ fn do_gc(
}

#[test]
#[cfg(not(feature = "test-engines-panic"))]
Copy link
Member

Choose a reason for hiding this comment

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

Why not test-engine-kv-rocksdb. The problem with test-engines-panic is, the build will still fail if we uses --features test-engine-kv-panic,test-engine-raft-rocksdb or --features test-engines-another-kind-of-engine.

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 seems I can't express that way. When ENABLE_FEATURES is not set by default, config predicate feature=test-engine-kv-rocksdb won't be satisfied either.

@BornChanger
*: address comments
f41270a 
Signed-off-by: BornChanger <dawn_catcher@126.com>
@BornChanger
*: code format
71547be 
Signed-off-by: BornChanger <dawn_catcher@126.com>
@tabokie
revert some order changes
e2aa88a 
Signed-off-by: tabokie <xy.tao@outlook.com>
@tabokie
Merge branch 'master' of https://github.com/tikv/tikv into gc_multi_r…
0541fbb 
…ocksdb

Signed-off-by: tabokie <xy.tao@outlook.com>
@tabokie
clean some more
3e5f6dc 
Signed-off-by: tabokie <xy.tao@outlook.com>
@tabokie
change feature flag
0a02388 
Signed-off-by: tabokie <xy.tao@outlook.com>
@tabokie
remove feature gates
43f252b 
Signed-off-by: tabokie <xy.tao@outlook.com>
@tabokie
major overhaul
d44bcfc 
Signed-off-by: tabokie <xy.tao@outlook.com>
@tabokie
clean up test
d928020 
Signed-off-by: tabokie <xy.tao@outlook.com>
@tabokie
Merge branch 'master' of https://github.com/tikv/tikv into gc_multi_r…
c10f9a0 
…ocksdb

Signed-off-by: tabokie <xy.tao@outlook.com>
@tabokie
clean up useless changes
a5cac39 
Signed-off-by: tabokie <xy.tao@outlook.com>
@tabokie
revert factory constructor changes
f365a0f 
Signed-off-by: tabokie <xy.tao@outlook.com>
@tabokie tabokie requested a review from BusyJay November 3, 2022 03:52
@tabokie
Merge branch 'master' into gc_multi_rocksdb
4e56f4a 
Signed-off-by: Xinye Tao <xy.tao@outlook.com>
@ti-chi-bot
Copy link
Member

@BornChanger: PR needs rebase.

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 kubernetes/test-infra repository.

@tabokie
Copy link
Member

tabokie commented Mar 1, 2023

Superseded by #13982.

@tabokie tabokie closed this Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tabokie tabokie tabokie left review comments

@SpadeA-Tang SpadeA-Tang SpadeA-Tang left review comments

@tonyxuqqi tonyxuqqi tonyxuqqi approved these changes

@BusyJay BusyJay Awaiting requested review from BusyJay

@Connor1996 Connor1996 Awaiting requested review from Connor1996

Assignees
No one assigned
Labels
contribution Type: PR - From contributors needs-rebase release-note-none size/XL status/LGT1 Status: PR - There is already 1 approval
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GC with compaction filter support multi tablets
6 participants
@BornChanger @ti-chi-bot @tabokie @BusyJay @tonyxuqqi @SpadeA-Tang