br: record files into filesOfPhysicalID #60423

Leavrth · 2025-04-07T10:38:03Z

What problem does this PR solve?

Issue Number: ref #60374

Problem Summary:
BR needs to update the stats meta of the partition.

What changed and how does it work?

Record files based on the physical ID.

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

Signed-off-by: Jianjun Liao <[email protected]>

tiprow · 2025-04-07T10:38:17Z

Hi @Leavrth. 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.

Signed-off-by: Jianjun Liao <[email protected]>

codecov · 2025-04-21T02:47:37Z

Codecov Report

Attention: Patch coverage is 75.38462% with 32 lines in your changes missing coverage. Please review.

Project coverage is 74.9570%. Comparing base (2704b09) to head (e43072a).
Report is 5 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #60423        +/-   ##
================================================
+ Coverage   73.1369%   74.9570%   +1.8200%     
================================================
  Files          1716       1762        +46     
  Lines        475676     483968      +8292     
================================================
+ Hits         347895     362768     +14873     
+ Misses       106410      98491      -7919     
- Partials      21371      22709      +1338

Flag	Coverage Δ
integration	`48.8257% <75.3846%> (?)`
unit	`72.4572% <18.4615%> (+0.0991%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.6553% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`62.3604% <75.3846%> (+14.9232%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Jianjun Liao <[email protected]>

br/pkg/task/restore.go

BornChanger · 2025-04-21T06:26:51Z

br/pkg/utils/misc.go

 }

+// GetPartitionByName gets the partition ID from the given tableInfo if its name matches
+func GetPartitionByName(tableInfo *model.TableInfo, name ast.CIStr) (int64, error) {


is there no exiting method to get partition by name?

Yes. Replace it by (*Partition).GetPartitionIDByName and reserve the function to convert the returned result -1 to error.

BornChanger

other part lgtm

Signed-off-by: Jianjun Liao <[email protected]>

BornChanger · 2025-04-21T10:56:14Z

br/pkg/task/restore.go

 	if err != nil {
 		return errors.Trace(err)
 	}
+	if backupMeta.IsRawKv || backupMeta.IsTxnKv {


please make a case (ut or integration) to cover the logic branch.

Signed-off-by: Jianjun Liao <[email protected]>

3pointer

rest LGTM

3pointer · 2025-04-21T05:47:52Z

br/pkg/task/restore.go


-	codec := mgr.GetStorage().GetCodec()
-	if len(files) == 0 {
+	anyFileKey := getAnyFileKeyFromTables(tables)


will file.StartKey be nil?

done, getAnyFileKeyFromTables will return a non-nil key.

Signed-off-by: Jianjun Liao <[email protected]>

Tristan1900

lgtm!

Tristan1900 · 2025-04-22T21:40:22Z

br/pkg/metautil/metafile.go

 				if fileMap != nil {
-					if files, ok := fileMap[table.Info.ID]; ok {
-						table.Files = append(table.Files, files...)
+					if files, ok := fileMap[table.Info.ID]; ok && len(files) > 0 {


I guess on line 443 it's not tableID but physcialID?

fileMap[tableID] = append(fileMap[tableID], file)

Tristan1900 · 2025-04-22T21:46:10Z

br/pkg/restore/snap_client/pipeline_items.go

+							return errors.Trace(err)
+						}
+						if statsErr = statsHandler.SaveMetaToStorage(newDefID, count, 0, "br restore", false); statsErr != nil {
+							log.Error("update stats meta failed", zap.Any("table", tbl.Table), zap.Int64("downstream partition id", newDefID), zap.Error(statsErr))


would log entire table be too much details? I'm concerned of log spamming

Tristan1900 · 2025-04-22T22:30:03Z

br/pkg/utils/misc.go

+	if tableInfo.Partition == nil {
+		return 0, errors.Errorf("the table %s[id=%d] does not have parition", tableInfo.Name.O, tableInfo.ID)
+	}
+	if partID := tableInfo.Partition.GetPartitionIDByName(name.L); partID > 0 {


could this name be empty string? Jus asking

Signed-off-by: Jianjun Liao <[email protected]>

Leavrth · 2025-04-23T07:28:57Z

/retest

tiprow · 2025-04-23T07:29:20Z

@Leavrth: 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.

ti-chi-bot · 2025-04-23T07:53:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 3pointer, BornChanger

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~br/OWNERS~~ [3pointer,BornChanger]

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

ti-chi-bot · 2025-04-23T07:53:10Z

[LGTM Timeline notifier]

Timeline:

2025-04-22 08:08:29.450046412 +0000 UTC m=+343053.261836781: ☑️ agreed by 3pointer.
2025-04-23 07:53:09.816662501 +0000 UTC m=+428533.628452874: ☑️ agreed by BornChanger.

Leavrth · 2025-04-23T07:54:05Z

/ok-to-test

Leavrth · 2025-04-23T09:24:09Z

/retest

Leavrth · 2025-04-23T11:07:35Z

/retest

ref pingcap#60374

record files into filesOfPhysicalID

c30c3cd

Signed-off-by: Jianjun Liao <[email protected]>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-tests-checked size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/needs-tests-checked labels Apr 7, 2025

Leavrth added 2 commits April 8, 2025 14:55

Merge branch 'master' into files_of_physical

52991a3

uploads partition stats meta

4e8db73

Signed-off-by: Jianjun Liao <[email protected]>

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 8, 2025

add integration test

fb07a46

Signed-off-by: Jianjun Liao <[email protected]>

ti-chi-bot 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 Apr 11, 2025

Leavrth mentioned this pull request Apr 18, 2025

enhancements when BR update stats meta if tables have no statistic data #60374

Closed

3 tasks

Merge branch 'master' into files_of_physical

c075238

Leavrth added 2 commits April 21, 2025 11:18

clean codes

a72e582

Signed-off-by: Jianjun Liao <[email protected]>

fix integration test

2e62d8a

Signed-off-by: Jianjun Liao <[email protected]>

BornChanger reviewed Apr 21, 2025

View reviewed changes

br/pkg/task/restore.go Outdated Show resolved Hide resolved

BornChanger reviewed Apr 21, 2025

View reviewed changes

Leavrth added 3 commits April 21, 2025 14:37

fix integration test

1bb76b8

Signed-off-by: Jianjun Liao <[email protected]>

commit some suggestions

4ef0c88

Signed-off-by: Jianjun Liao <[email protected]>

fix integration test

447d740

Signed-off-by: Jianjun Liao <[email protected]>

Leavrth mentioned this pull request Apr 21, 2025

br: introduce a parameter --auto-analyze to trigger tidb_analyze_priority_queue to analyze tables in the background. #60687

Merged

13 tasks

BornChanger reviewed Apr 21, 2025

View reviewed changes

Leavrth added 2 commits April 22, 2025 13:47

commit some suggestions

9b12f37

Signed-off-by: Jianjun Liao <[email protected]>

fix integration test

3160fa5

Signed-off-by: Jianjun Liao <[email protected]>

3pointer approved these changes Apr 22, 2025

View reviewed changes

ti-chi-bot bot added the approved label Apr 22, 2025

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Apr 22, 2025

Leavrth added 2 commits April 22, 2025 17:37

commit some suggestions

1c28939

Signed-off-by: Jianjun Liao <[email protected]>

fix integration test

a69a2a6

Signed-off-by: Jianjun Liao <[email protected]>

Tristan1900 reviewed Apr 22, 2025

View reviewed changes

Leavrth added 2 commits April 23, 2025 14:22

commit some suggestions

8aa859c

Signed-off-by: Jianjun Liao <[email protected]>

Merge branch 'master' into files_of_physical

e43072a

BornChanger approved these changes Apr 23, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 23, 2025

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Apr 23, 2025

ti-chi-bot bot merged commit 5a03265 into pingcap:master Apr 23, 2025
34 checks passed

Kiran01bm pushed a commit to Kiran01bm/tidb that referenced this pull request Apr 29, 2025

br: record files into filesOfPhysicalID (pingcap#60423)

44981f2

ref pingcap#60374

Tristan1900 pushed a commit to Tristan1900/tidb that referenced this pull request Jun 6, 2025

br: record files into filesOfPhysicalID (pingcap#60423)

6cc61e3

ref pingcap#60374

Leavrth mentioned this pull request Jun 6, 2025

br: cherry pick to hot fix release 8.5 20250114 v8.5.0 second batch | tidb-test=release-8.5.1 #61534

Merged

13 tasks

br: record files into filesOfPhysicalID #60423

br: record files into filesOfPhysicalID #60423

Uh oh!

Conversation

Leavrth commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

tiprow bot commented Apr 7, 2025

Uh oh!

codecov bot commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BornChanger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

3pointer 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!

Tristan1900 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!

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!

Leavrth commented Apr 23, 2025

Uh oh!

tiprow bot commented Apr 23, 2025

Uh oh!

ti-chi-bot bot commented Apr 23, 2025

Uh oh!

ti-chi-bot bot commented Apr 23, 2025

[LGTM Timeline notifier]

Uh oh!

Leavrth commented Apr 23, 2025

Uh oh!

Leavrth commented Apr 23, 2025

Uh oh!

Leavrth commented Apr 23, 2025

Uh oh!

Uh oh!

Uh oh!

Leavrth commented Apr 7, 2025 •

edited

Loading

codecov bot commented Apr 21, 2025 •

edited

Loading