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 62.03606% with 358 lines in your changes missing coverage. Please review.

Project coverage is 74.1448%. Comparing base (67249cb) to head (998ca37).
Report is 9 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57394        +/-   ##
================================================
+ Coverage   73.0858%   74.1448%   +1.0590%     
================================================
  Files          1676       1695        +19     
  Lines        463685     464167       +482     
================================================
+ Hits         338888     344156      +5268     
+ Misses       103941      98406      -5535     
- Partials      20856      21605       +749

Flag	Coverage Δ
integration	`40.0383% <62.0360%> (?)`

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`60.6168% <62.0360%> (+14.8958%)`	⬆️

ti-chi-bot · 2025-01-01T23:59:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign leavrth for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

br/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

br/pkg/utils/schema.go

br/cmd/br/cmd.go

br/pkg/stream/rewrite_meta_rawkv.go

br/tests/config/tikv.toml

br/pkg/stream/table_mapping.go

Leavrth · 2025-01-03T06:05:45Z

br/pkg/stream/table_mapping.go

 	} else {
 		dr.Name = dbInfo.Name.O
 	}
 	return nil
 }

-func (tc *TableMappingManager) parseTableValueAndUpdateIdMapping(dbID int64, value []byte) error {
+func (tm *TableMappingManager) ProcessTableValueAndUpdateIdMapping(dbID int64, tableInfo model.TableInfo) error {


Why not pass by pointer, which is like tableInfo *model.TableInfo?

And remove tableInfo.ID = tableReplace.TableID and partitions.Definitions[i].ID = newID below.

good call, these are from the rewrite method, not needed here

br/pkg/stream/table_mapping.go

br/pkg/task/stream.go

br/pkg/restore/log_client/client.go

Leavrth · 2025-01-03T16:39:47Z

br/pkg/restore/log_client/batch_meta_processor.go

+			}
+
+			// collect table history indexed by table id, same id may have different table names in history
+			if meta.IsTableKey(rawKey.Field) {


else if meta.IsTableKey(rawKey.Field) {

Leavrth · 2025-01-03T16:47:09Z

br/pkg/stream/table_mapping.go

 	} else {
 		dr.Name = dbInfo.Name.O
 	}
 	return nil
 }

-func (tc *TableMappingManager) parseTableValueAndUpdateIdMapping(dbID int64, value []byte) error {
+func (tm *TableMappingManager) ProcessTableValueAndUpdateIdMapping(dbID int64, tableInfo model.TableInfo) error {


And remove tableInfo.ID = tableReplace.TableID and partitions.Definitions[i].ID = newID below.

Leavrth · 2025-01-03T17:06:00Z

br/pkg/task/restore.go

+			// check during log backup
+			dbName = name
+		} else {
+			log.Warn("did not find db id in full/log backup, "+


perhaps it's better to return an error because it must be a code-level error.

if we have full backup with --filter="foo*" and PiTR doesn't have any filter, it can have this problem, let me know your thoughts on how to better handle this situation

Leavrth

rest LGTM

Leavrth · 2025-01-06T03:19:55Z

br/pkg/task/restore.go

+	return
+}
+
+func adjustTablesToRestoreAndCreateTableTracker(


the function is important so it needs some unit tests.

Leavrth · 2025-01-06T03:41:36Z

br/pkg/stream/table_mapping.go

+		tempIDs = append(tempIDs, id)
+	}
+
+	// sort to -1, -2, -4, -8 ... etc


tempIDs[i] > tempIDs[j] does not means upstream(tempIDs[i]) > upstream[tempIDs[j]), so is it necessary to sort tempIDs?

yes it doesn't, I was thinking to assign id based on first seen first, somewhat like sorted by time in the log backup, I was hoping it can be a bit useful when debugging. The order doesn't suggest anything such as upstream id ordering.

Leavrth · 2025-01-06T03:46:05Z

br/pkg/task/restore.go

+			}
+
+			// need to restore the matching table in snapshot restore phase
+			for _, originalTable := range originalDB.Tables {


I prefer to merge filterRestoreFiles and adjustTablesToRestoreAndCreateTableTracker without any intermediates. That's because I am worried about the time complexity of the loop.

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

ti-chi-bot · 2025-01-09T01:28:05Z

@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	`998ca37`	link	true	`/test unit-test`
pull-br-integration-test	`998ca37`	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 3 times, most recently from 04d4ccf to f0eb49d Compare January 1, 2025 23:58

Tristan1900 force-pushed the table-filter branch 3 times, most recently from 2f06bd9 to 924a195 Compare January 2, 2025 01:32

Leavrth reviewed Jan 3, 2025

View reviewed changes

br/pkg/utils/schema.go Outdated Show resolved Hide resolved

br/cmd/br/cmd.go Outdated Show resolved Hide resolved

br/pkg/stream/rewrite_meta_rawkv.go Outdated Show resolved Hide resolved

br/tests/config/tikv.toml Outdated Show resolved Hide resolved

Leavrth reviewed Jan 3, 2025

View reviewed changes

Tristan1900 force-pushed the table-filter branch from ee5d347 to 45bcdca Compare January 3, 2025 21:53

Leavrth reviewed Jan 6, 2025

View reviewed changes

Tristan1900 added 6 commits January 7, 2025 20:52

initial

f987c94

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

add one more unit test, and some minor fixes

8fd492e

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

remove filter and use tracker

1c4c1af

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

address comments

2e02d55

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

fix bazel

257c287

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

address comments

9bf682b

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

Tristan1900 force-pushed the table-filter branch from 4788a14 to 9bf682b Compare January 8, 2025 01:58

Tristan1900 added 2 commits January 8, 2025 17:13

style fix

1c7ce5a

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

fix test

998ca37

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

ti-chi-bot bot commented Jan 1, 2025

Leavrth Jan 3, 2025

Leavrth Jan 3, 2025

Tristan1900 Jan 7, 2025

Leavrth Jan 3, 2025

Leavrth Jan 3, 2025

Leavrth Jan 3, 2025

Tristan1900 Jan 7, 2025

Leavrth left a comment

Leavrth Jan 6, 2025

Leavrth Jan 6, 2025

Tristan1900 Jan 7, 2025

Leavrth Jan 6, 2025

ti-chi-bot bot commented Jan 9, 2025

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

ti-chi-bot bot commented Jan 1, 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

Leavrth left a 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 9, 2025

Tristan1900 commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading