br: add table filter for log restore #57394

Tristan1900 · 2024-11-15T01:19:07Z

What problem does this PR solve?

Issue Number: close #57613

Problem Summary:
Need table filter for PiTR

What changed and how does it work?

The following happens if a custom filter is specified during PiTR.

Do a log backup meta scan at first before snapshot restore, and record all the table rename history and table mapping information during the scan, since we don't have access to genGlobalId, we use a temp id as a placeholder.
During snapshot restore, adjust the tables that need to be restored according to the rename history. It needs to add tables that renamed into the filter and removes the tables that are renamed out of the filter range during log backup. And put all the tables that need to be restored into a map/filter and pass down to log restore
During log restore, we use that map/filter from the above and filters out the tables/kvs that don't need to restore/rewrite, and we replace the temp id in the id map with real global ids.
For actual meta kv restore, use id map to filter out the kvs that doesn't need to restore. so for tables not exist in the id map, we can simply ignore it because they are filtered out.
The id map is persisted once built to work with checkpoint, so our filter naturally works with checkpoint as well.

Performance:
we are still scanning log meta kv twice, it's just we move the previously id map building step before snapshot happens.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

ti-chi-bot · 2024-11-15T01:19:12Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tiprow · 2024-11-15T01:19:25Z

Hi @Tristan1900. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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-sigs/prow repository.

codecov · 2024-11-25T01:03:05Z

Codecov Report

Attention: Patch coverage is 66.00634% with 429 lines in your changes missing coverage. Please review.

Project coverage is 74.1069%. Comparing base (7cee190) to head (977507c).
Report is 11 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57394        +/-   ##
================================================
+ Coverage   73.1112%   74.1069%   +0.9957%     
================================================
  Files          1684       1709        +25     
  Lines        466366     479249     +12883     
================================================
+ Hits         340966     355157     +14191     
+ Misses       104467     101489      -2978     
- Partials      20933      22603      +1670

Flag	Coverage Δ
integration	`41.5512% <66.0063%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`61.0781% <66.0063%> (+15.7548%)`	⬆️

Tristan1900 · 2025-01-13T20:21:59Z

/retest

tiprow · 2025-01-13T20:22:27Z

@Tristan1900: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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-sigs/prow repository.

YuJuncen · 2025-01-14T03:39:57Z

br/pkg/restore/log_client/batch_meta_processor.go

+		}
+
+		// write cf doesn't have short value in it
+		if value == nil {


Should we handle the DELETE operations in the write CF? (Which doesn't have a value.)

right, I think it's not necessarily needed, we are only tracking the all the table ids at this step, if there is a delete there must be a create table happened before, so that should be tracked already.

YuJuncen · 2025-01-14T03:57:09Z

br/pkg/restore/log_client/client.go

 	ctx context.Context,
 	s storage.ExternalStorage,
-	tableFilter filter.Filter,
+	piTRTableFilter *utils.PiTRTableTracker,


Perhaps name the variable piTRTableTracker or rename the PiTRTableTracker to something like TracedPiTRTableFilter? As tracker.ContainsDB(foo) -> Filtered Out looks a little bit strange.

agree, I want to avoid using filter here because I feel like the term is overloaded so can cause confusion. let me rename all to be tracker related. but actually tracker.ContainsDB(foo) -> included

YuJuncen · 2025-01-14T06:09:02Z

br/pkg/restore/snap_client/client.go

+func (rc *SnapClient) GetDatabaseMap() map[int64]*metautil.Database {
+	dbMap := make(map[int64]*metautil.Database)
+	for _, db := range rc.databases {
+		dbMap[db.Info.ID] = db
+	}
+	return dbMap


How about maps.Clone?

oh it's indexed differently if you mean to clone rc.databases. one is indexed by db name and this one is db id

YuJuncen · 2025-01-14T07:18:29Z

br/pkg/task/restore.go

@@ -1655,3 +1823,26 @@ func afterTableRestoredCh(ctx context.Context, createdTables []*snapclient.Creat
 	}()
 	return outCh
 }
+
+func convertMapsToSlices(


How about make a generic function like:

func convertMapToSlice[K, V any](m map[K]V) []V { values := make([]T, 0, len(m)) for v := range maps.Values(m) { values = append(values, v) } return values }

And call it three times?

YuJuncen · 2025-01-15T05:57:10Z

br/pkg/task/restore.go

+			}
+			// handle case where current is not in range and original was in range, we need to remove the original from
+			// restoring
+		} else if utils.MatchTable(cfg.TableFilter, dbName, start.TableName) {


Just wondering whether the table will be created again by putting meta again (By the meta entry of rename). I didn't found something that filters out not matched meta keys.

ok not sure about the meaning of this one, will follow up offline

YuJuncen · 2025-01-15T06:42:50Z

br/pkg/task/restore_test.go

+		},
+	}
+
+	// Test case 1: Basic table tracking


When there are many similar cases, would you parameterize those case? like:

type Case struct { Filter string PiTRHistories struct{ tid int64, tname string, did int64 } ExceptsTables [][2]int ExceptsDBNotExist []int ExcpetsTableNotExist []int }

Also you may warp each case with t.Run, so cases can be run independently.

YuJuncen · 2025-01-15T06:46:35Z

br/pkg/task/stream.go

 // gc.ratio-threshold = "-1.0", which represents disable gc in TiKV.
-func KeepGcDisabled(g glue.Glue, store kv.Storage) (RestoreFunc, string, error) {
+func DisableGc(g glue.Glue, store kv.Storage) (RestoreGcFunc, string, error) {


Go prefers captialize all letters in acronyms. As we are going to rename we may follow this tradition by the way.

Suggested change

func DisableGc(g glue.Glue, store kv.Storage) (RestoreGcFunc, string, error) {

func DisableGC(g glue.Glue, store kv.Storage) (RestoreGCFunc, string, error) {

got it, good to know!

Signed-off-by: Wenqi Mou <[email protected]>

ti-chi-bot · 2025-01-23T21:59:28Z

@Tristan1900: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
idc-jenkins-ci-tidb/unit-test	`977507c`	link	true	`/test unit-test`
idc-jenkins-ci-tidb/check_dev	`977507c`	link	true	`/test check-dev`
pull-br-integration-test	`977507c`	link	true	`/test pull-br-integration-test`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

Tristan1900 force-pushed the table-filter branch from 7c04493 to 8ef978b Compare November 22, 2024 01:21

ti-chi-bot bot removed do-not-merge/needs-tests-checked do-not-merge/needs-linked-issue labels Nov 22, 2024

Tristan1900 force-pushed the table-filter branch from 8ef978b to 02a2318 Compare November 25, 2024 00:04

Tristan1900 marked this pull request as ready for review November 25, 2024 00:06

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

Tristan1900 force-pushed the table-filter branch from 02a2318 to c29b578 Compare November 25, 2024 00:18

Tristan1900 force-pushed the table-filter branch 2 times, most recently from f624281 to 6ebc531 Compare November 25, 2024 15:53

Tristan1900 force-pushed the table-filter branch 6 times, most recently from 852dcff to b5132a9 Compare December 23, 2024 18:56

Tristan1900 force-pushed the table-filter branch 7 times, most recently from d2b68b8 to d72415c Compare December 30, 2024 23:30

Tristan1900 force-pushed the table-filter branch 2 times, most recently from 021494a to 1eee968 Compare January 13, 2025 17:37

Tristan1900 force-pushed the table-filter branch from 37f61af to 9b515fe Compare January 15, 2025 00:00

YuJuncen reviewed Jan 15, 2025

View reviewed changes

Tristan1900 force-pushed the table-filter branch from 9b515fe to f6d3330 Compare January 22, 2025 00:52

Tristan1900 added 14 commits January 22, 2025 13:40

initial

1e18536

Signed-off-by: Wenqi Mou <[email protected]>

add one more unit test, and some minor fixes

720290f

Signed-off-by: Wenqi Mou <[email protected]>

remove filter and use tracker

e6b4182

Signed-off-by: Wenqi Mou <[email protected]>

address comments

e8ea853

Signed-off-by: Wenqi Mou <[email protected]>

fix bazel

0b039db

Signed-off-by: Wenqi Mou <[email protected]>

address comments

aa073a1

Signed-off-by: Wenqi Mou <[email protected]>

style fix

3052a62

Signed-off-by: Wenqi Mou <[email protected]>

fix test

8ca6342

Signed-off-by: Wenqi Mou <[email protected]>

fix test

10754a6

Signed-off-by: Wenqi Mou <[email protected]>

minor fix and tests

2da96ba

Signed-off-by: Wenqi Mou <[email protected]>

fix int test

7da4988

Signed-off-by: Wenqi Mou <[email protected]>

fix base merge

4463bd0

Signed-off-by: Wenqi Mou <[email protected]>

fix exchange partition, address comments

b6c7dea

Signed-off-by: Wenqi Mou <[email protected]>

fix sql, some rename

6940958

Signed-off-by: Wenqi Mou <[email protected]>

Tristan1900 force-pushed the table-filter branch from 0e3162c to 6940958 Compare January 22, 2025 18:40

Tristan1900 added 7 commits January 23, 2025 12:21

add tests

8fe0cc8

Signed-off-by: Wenqi Mou <[email protected]>

make it safe by return error instead of nil

ea73544

Signed-off-by: Wenqi Mou <[email protected]>

update table mapping

326f4fb

Signed-off-by: Wenqi Mou <[email protected]>

add unit test, fix format

f11bb1d

Signed-off-by: Wenqi Mou <[email protected]>

fix bazel

c3be5c0

Signed-off-by: Wenqi Mou <[email protected]>

minor fix

2854ff7

Signed-off-by: Wenqi Mou <[email protected]>

merge rewrite fix in the other pr and adjust

977507c

Signed-off-by: Wenqi Mou <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br: add table filter for log restore #57394

br: add table filter for log restore #57394

Tristan1900 commented Nov 15, 2024 •

edited

Loading

ti-chi-bot bot commented Nov 15, 2024

tiprow bot commented Nov 15, 2024

codecov bot commented Nov 25, 2024 •

edited

Loading

Tristan1900 commented Jan 13, 2025

tiprow bot commented Jan 13, 2025

YuJuncen Jan 14, 2025

Tristan1900 Jan 21, 2025

YuJuncen Jan 14, 2025

Tristan1900 Jan 21, 2025

YuJuncen Jan 14, 2025

Tristan1900 Jan 22, 2025

YuJuncen Jan 14, 2025

YuJuncen Jan 15, 2025

Tristan1900 Jan 22, 2025

YuJuncen Jan 15, 2025

Tristan1900 Jan 22, 2025

YuJuncen Jan 15, 2025

Tristan1900 Jan 22, 2025

ti-chi-bot bot commented Jan 23, 2025

	func DisableGc(g glue.Glue, store kv.Storage) (RestoreGcFunc, string, error) {
	func DisableGC(g glue.Glue, store kv.Storage) (RestoreGCFunc, string, error) {

br: add table filter for log restore #57394

Are you sure you want to change the base?

br: add table filter for log restore #57394

Conversation

Tristan1900 commented Nov 15, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Nov 15, 2024

tiprow bot commented Nov 15, 2024

codecov bot commented Nov 25, 2024 • edited Loading

Codecov Report

Tristan1900 commented Jan 13, 2025

tiprow bot commented Jan 13, 2025

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

ti-chi-bot bot commented Jan 23, 2025

Tristan1900 commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading