br: clean codes -- restore tables #55519

Leavrth · 2024-08-20T05:44:51Z

What problem does this PR solve?

Issue Number: ref #52877

Problem Summary:
we can clean some codes

What changed and how does it work?

simplify the split & scatter and download & ingest stages

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 · 2024-08-20T05:45:10Z

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 · 2024-08-20T09:16:46Z

Codecov Report

Attention: Patch coverage is 74.45652% with 94 lines in your changes missing coverage. Please review.

Project coverage is 75.7049%. Comparing base (c1c74b1) to head (54d72eb).
Report is 37 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #55519        +/-   ##
================================================
+ Coverage   72.8739%   75.7049%   +2.8310%     
================================================
  Files          1576       1585         +9     
  Lines        440775     454817     +14042     
================================================
+ Hits         321210     344319     +23109     
+ Misses        99821      89552     -10269     
- Partials      19744      20946      +1202

Flag	Coverage Δ
integration	`50.4126% <64.1304%> (?)`
unit	`71.8581% <30.9782%> (-1.0150%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.9567% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`62.2083% <74.4565%> (+16.2624%)`	⬆️

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

Leavrth · 2024-08-23T03:13:21Z

br/pkg/rtree/rtree.go

@@ -85,8 +84,67 @@ func (rg *Range) Less(than btree.Item) bool {
 	return bytes.Compare(rg.StartKey, ta.StartKey) < 0
 }

+var _ btree.Item = &RangeStats{}


Range for backup, RangeStats for restore to be merged

Leavrth · 2024-08-23T03:15:24Z

br/pkg/task/restore.go

 	defer updateCh.Close()
-	sender, err := snapclient.NewTiKVSender(ctx, client, updateCh, cfg.PDConcurrency)
+	placementRuleManager, err := snapclient.NewPlacementRuleManager(ctx, mgr.GetPDClient(), mgr.GetPDHTTPClient(), mgr.GetTLSConfig(), cfg.Online)


There are many functions that set something before split&scatter and download&ingest and reset after that. We can unify them into one context manager.

Leavrth · 2024-08-23T03:19:12Z

br/pkg/restore/snap_client/batcher.go

@@ -324,133 +114,29 @@ func (b *Batcher) filterOutRanges(checkpointSet map[string]struct{}, drained []r
 // |--|-------|
 // |t2|t3     |
 // as you can see, all restored ranges would be removed.
-func (b *Batcher) drainRanges() DrainResult {
+func drainRanges(


will be removed by the next PR

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

Leavrth · 2024-08-23T05:08:32Z

/retest

tiprow · 2024-08-23T05:08:53Z

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

YuJuncen

rest lgtm

br/pkg/rtree/rtree.go

br/pkg/task/restore_raw.go

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

Leavrth · 2024-08-26T01:27:22Z

/retest

tiprow · 2024-08-26T01:27:42Z

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

3pointer · 2024-08-26T04:15:35Z

br/tests/br_small_batch_size/run.sh

@@ -1,81 +0,0 @@
-#!/bin/sh


This test is used for small tables scenario test. Shall we keep it?

3pointer · 2024-08-26T05:21:34Z

br/pkg/restore/utils/merge.go

@@ -48,6 +51,14 @@ func MergeAndRewriteFileRanges(
 	for _, file := range files {
 		filesMap[string(file.StartKey)] = append(filesMap[string(file.StartKey)], file)

+		// Assert that it has the same end key.
+		if !bytes.Equal(filesMap[string(file.StartKey)][0].EndKey, file.EndKey) {


why need this check? for some scenarios like compacted ssts may not satisfied.

To assert range's start-key and end-key is the same as that of any file of the range.

3pointer · 2024-08-26T05:38:46Z

br/pkg/rtree/rtree.go

+// RangeStats represents a restore merge result.
+type RangeStats struct {
+	Range
+	Size  uint64


Size and Count seems never used...

Used in the next PR

ti-chi-bot · 2024-08-26T05:41:54Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,YuJuncen]

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 · 2024-08-26T05:41:57Z

[LGTM Timeline notifier]

Timeline:

2024-08-23 08:24:18.440102219 +0000 UTC m=+513053.574552340: ☑️ agreed by YuJuncen.
2024-08-26 05:41:56.407561151 +0000 UTC m=+762511.542011272: ☑️ agreed by 3pointer.

Leavrth · 2024-08-26T06:26:15Z

/ok-to-test

merge ranges -- restore tables

cc2337e

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

ti-chi-bot bot added do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed do-not-merge/needs-tests-checked labels Aug 20, 2024

Leavrth mentioned this pull request Aug 20, 2024

Snapshot Restore: CoarseGrained completely replaces FineGrained #52877

Closed

3 tasks

Leavrth added 2 commits August 20, 2024 16:34

Merge branch 'master' into merge_ranges_restoretables

40c4035

resolve conflicts

8dfbd61

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

resolve conflicts

b4cb9ed

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

Leavrth requested review from 3pointer and YuJuncen August 20, 2024 10:20

fix unit test

106fd5c

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

Leavrth commented Aug 23, 2024

View reviewed changes

fix typos

b918f48

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

YuJuncen approved these changes Aug 23, 2024

View reviewed changes

br/pkg/rtree/rtree.go Outdated Show resolved Hide resolved

br/pkg/rtree/rtree.go Outdated Show resolved Hide resolved

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

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 23, 2024

Leavrth added 2 commits August 23, 2024 17:14

commit some suggestions

4c9d48f

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

fix typos

54d72eb

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

3pointer reviewed Aug 26, 2024

View reviewed changes

3pointer approved these changes Aug 26, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 26, 2024

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Aug 26, 2024

ti-chi-bot bot merged commit 4eeeef8 into pingcap:master Aug 26, 2024
32 checks passed

Leavrth mentioned this pull request Aug 27, 2024

br: restore merge split ranges with small data #55104

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br: clean codes -- restore tables #55519

br: clean codes -- restore tables #55519

Leavrth commented Aug 20, 2024 •

edited

Loading

tiprow bot commented Aug 20, 2024

codecov bot commented Aug 20, 2024 •

edited

Loading

Leavrth Aug 23, 2024

Leavrth Aug 23, 2024

Leavrth Aug 23, 2024

Leavrth commented Aug 23, 2024

tiprow bot commented Aug 23, 2024

YuJuncen left a comment

Leavrth commented Aug 26, 2024

tiprow bot commented Aug 26, 2024

3pointer Aug 26, 2024 •

edited

Loading

3pointer Aug 26, 2024

Leavrth Aug 26, 2024

3pointer Aug 26, 2024

Leavrth Aug 26, 2024

ti-chi-bot bot commented Aug 26, 2024

ti-chi-bot bot commented Aug 26, 2024

Leavrth commented Aug 26, 2024

br: clean codes -- restore tables #55519

br: clean codes -- restore tables #55519

Conversation

Leavrth commented Aug 20, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Aug 20, 2024

codecov bot commented Aug 20, 2024 • edited Loading

Codecov Report

Leavrth Aug 23, 2024

Choose a reason for hiding this comment

Leavrth Aug 23, 2024

Choose a reason for hiding this comment

Leavrth Aug 23, 2024

Choose a reason for hiding this comment

Leavrth commented Aug 23, 2024

tiprow bot commented Aug 23, 2024

YuJuncen left a comment

Choose a reason for hiding this comment

Leavrth commented Aug 26, 2024

tiprow bot commented Aug 26, 2024

3pointer Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

3pointer Aug 26, 2024

Choose a reason for hiding this comment

Leavrth Aug 26, 2024

Choose a reason for hiding this comment

3pointer Aug 26, 2024

Choose a reason for hiding this comment

Leavrth Aug 26, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Aug 26, 2024

ti-chi-bot bot commented Aug 26, 2024

[LGTM Timeline notifier]

Leavrth commented Aug 26, 2024

Leavrth commented Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 20, 2024 •

edited

Loading

3pointer Aug 26, 2024 •

edited

Loading