external: auto switch reader mode #46979

wjhuang2016 · 2023-09-14T09:04:35Z

What problem does this PR solve?

Issue Number: close #45719

Problem Summary:
Support switching read mode automatically so the speed does not degrade for the hot-point.

What is changed and how it works?

Record the file access stat, and switch reader mode if necessary.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

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: wjhuang2016 <huangwenjun1997@gmail.com>

codecov · 2023-09-14T09:16:18Z

Codecov Report

Merging #46979 (ffc1fa1) into master (59e31f0) will decrease coverage by 0.3388%.
The diff coverage is 91.6666%.

❗ Current head ffc1fa1 differs from pull request most recent head 984d3f6. Consider uploading reports for the commit 984d3f6 to get more accurate results

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #46979        +/-   ##
================================================
- Coverage   73.0821%   72.7434%   -0.3388%     
================================================
  Files          1336       1357        +21     
  Lines        398227     404816      +6589     
================================================
+ Hits         291033     294477      +3444     
- Misses        88451      91686      +3235     
+ Partials      18743      18653        -90

Flag	Coverage Δ
integration	`30.3402% <0.0000%> (?)`
unit	`73.1201% <91.6666%> (+0.0379%)`	⬆️

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

Components	Coverage Δ
dumpling	`53.9913% <ø> (ø)`
parser	`84.9980% <ø> (ø)`
br	`48.7498% <91.6666%> (-4.1971%)`	⬇️

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

lance6716

rest lgtm

lance6716 · 2023-09-14T09:27:04Z

br/pkg/lightning/backend/external/byte_reader.go

@@ -178,6 +178,7 @@ func (r *byteReader) reload() error {
 	to := r.useConcurrentReader.Load()
 	now := r.useConcurrentReaderCurrent.Load()
 	if to != now {
+		logutil.BgLogger().Info("switch reader mode", zap.Bool("use concurrent mode", to))


Suggested change

logutil.BgLogger().Info("switch reader mode", zap.Bool("use concurrent mode", to))

logutil.Logger(r.ctx).Info("switch reader mode", zap.Bool("use concurrent mode", to))

lance6716 · 2023-09-14T09:31:37Z

br/pkg/lightning/backend/external/iter_test.go

+	writer := NewWriterBuilder().
+		SetPropKeysDistance(100).
+		SetMemorySizeLimit(512*1024).
+		Build(st, "testprefix", strconv.Itoa(0))


Suggested change

Build(st, "testprefix", strconv.Itoa(0))

Build(st, "testprefix", "0"))

lance6716 · 2023-09-14T09:34:00Z

br/pkg/lightning/backend/external/kv_reader.go

@@ -42,7 +42,6 @@ func newKVReader(
 	}
 	br, err := newByteReader(ctx, sr, bufSize, store, name, false)
 	if err != nil {
-		br.Close()


where do we Close now?

lance6716 · 2023-09-14T09:37:55Z

br/pkg/lightning/backend/external/iter.go

@@ -198,6 +202,23 @@ func (i *mergeIter[T, R]) next() bool {
 	var zeroT T
 	i.curr = zeroT
 	if i.lastReaderIdx >= 0 {
+		cnt, ok := i.checkHotPointMap[i.lastReaderIdx]
+		if !ok {
+			i.checkHotPointMap[i.lastReaderIdx] = 1


we can rely on the zero value when map entry is not found, maybe

i.checkHotPointMap[i.lastReaderIdx] = i.checkHotPointMap[i.lastReaderIdx] + 1

lance6716 · 2023-09-14T09:40:08Z

br/pkg/lightning/backend/external/iter.go

+		if i.checkHotPointCnt == 1000 {
+			i.checkHotPointCnt = 0
+			for idx, cnt := range i.checkHotPointMap {
+				(*i.readers[idx]).setReadMode(cnt > 500)


seems byteReader is not concurrently used, we don;t need atomic.Bool to represent useConcurrentReader

Yes, #46924 works for it

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

br/pkg/lightning/backend/external/iter.go

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

Signed-off-by: lance6716 <lance6716@gmail.com>

…_merge_iter

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-09-14T10:49:40Z

/retest

lance6716 · 2023-09-14T11:21:18Z

/retest

lance6716 · 2023-09-14T11:21:21Z

/retest

tangenta · 2023-09-15T03:05:52Z

br/pkg/lightning/backend/external/iter.go

+	curr             T
+	lastReaderIdx    int
+	err              error
+	checkHotPointMap map[int]int


Maybe rename to hotspotMap?

I didn't understand the meaning of this name at first glance too.

tangenta

Rest LGTM

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

…_merge_iter

ti-chi-bot · 2023-09-18T04:12:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance6716, tangenta

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:

~~OWNERS~~ [lance6716,tangenta]

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 · 2023-09-18T04:12:44Z

[LGTM Timeline notifier]

Timeline:

2023-09-14 10:41:27.695329923 +0000 UTC m=+168453.662917963: ☑️ agreed by lance6716.
2023-09-18 04:12:43.189839893 +0000 UTC m=+490729.157427943: ☑️ agreed by tangenta.

wjhuang2016 · 2023-09-18T06:06:26Z

/test unit-test

tiprow · 2023-09-18T06:06:40Z

@wjhuang2016: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test unit-test

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.

wjhuang2016 · 2023-09-18T07:34:56Z

/test check-dev

tiprow · 2023-09-18T07:35:11Z

@wjhuang2016: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test check-dev

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.

wjhuang2016 · 2023-09-18T07:41:02Z

/test check-dev

tiprow · 2023-09-18T07:41:16Z

@wjhuang2016: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test check-dev

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.

ti-chi-bot · 2023-09-18T07:44:47Z

@wjhuang2016: The following test 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/check_dev	`ffc1fa1`	link	true	`/test check-dev`

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

…ge_iter

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

wjhuang2016 added 13 commits September 4, 2023 18:03

done

d996356

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

revert

64f9383

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

clean

8206c71

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

done

e060ea8

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

refine

7ff01fb

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

fix bazel

c364778

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

refine

4c8b2f2

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

refine

df59fe0

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

refine

5257527

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

refine

ee92218

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

fix bazel

3ca4827

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

save

be15364

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

fix

f47e13e

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 14, 2023

fix test

a08fa9b

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

lance6716 reviewed Sep 14, 2023

View reviewed changes

ti-chi-bot bot removed do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked labels Sep 14, 2023

done

870b18c

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

okJiang reviewed Sep 14, 2023

View reviewed changes

br/pkg/lightning/backend/external/iter.go Outdated Show resolved Hide resolved

okJiang reviewed Sep 14, 2023

View reviewed changes

br/pkg/lightning/backend/external/iter.go Outdated Show resolved Hide resolved

br/pkg/lightning/backend/external/iter.go Outdated Show resolved Hide resolved

wjhuang2016 and others added 3 commits September 14, 2023 18:38

fix

e29e0f5

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

fix UT

9c78ad8

Signed-off-by: lance6716 <lance6716@gmail.com>

Merge branch 'new_merge_iter' of github.com:wjhuang2016/tidb into new…

665cb2d

…_merge_iter

lance6716 approved these changes Sep 14, 2023

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Sep 14, 2023

ti-chi-bot bot added the approved label Sep 14, 2023

fix UT

78a58be

Signed-off-by: lance6716 <lance6716@gmail.com>

tangenta reviewed Sep 15, 2023

View reviewed changes

wjhuang2016 added 2 commits September 18, 2023 11:58

fix

d93a1ff

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

Merge branch 'new_merge_iter' of github.com:wjhuang2016/tidb into new…

ffc1fa1

…_merge_iter

tangenta approved these changes Sep 18, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 18, 2023

wjhuang2016 added 2 commits September 18, 2023 16:46

Merge branch 'master' of https://github.com/pingcap/tidb into new_mer…

291b720

…ge_iter

fix test

984d3f6

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

wjhuang2016 merged commit 3cd5dce into pingcap:master Sep 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

external: auto switch reader mode #46979

external: auto switch reader mode #46979

wjhuang2016 commented Sep 14, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading

lance6716 left a comment

lance6716 Sep 14, 2023

lance6716 Sep 14, 2023

lance6716 Sep 14, 2023

lance6716 Sep 14, 2023

lance6716 Sep 14, 2023

wjhuang2016 Sep 14, 2023

lance6716 commented Sep 14, 2023

lance6716 commented Sep 14, 2023

lance6716 commented Sep 14, 2023

tangenta Sep 15, 2023

okJiang Sep 15, 2023

tangenta left a comment

ti-chi-bot bot commented Sep 18, 2023

ti-chi-bot bot commented Sep 18, 2023

wjhuang2016 commented Sep 18, 2023

tiprow bot commented Sep 18, 2023

wjhuang2016 commented Sep 18, 2023

tiprow bot commented Sep 18, 2023

wjhuang2016 commented Sep 18, 2023

tiprow bot commented Sep 18, 2023

ti-chi-bot bot commented Sep 18, 2023

	logutil.BgLogger().Info("switch reader mode", zap.Bool("use concurrent mode", to))
	logutil.Logger(r.ctx).Info("switch reader mode", zap.Bool("use concurrent mode", to))

	Build(st, "testprefix", strconv.Itoa(0))
	Build(st, "testprefix", "0"))

external: auto switch reader mode #46979

external: auto switch reader mode #46979

Conversation

wjhuang2016 commented Sep 14, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

codecov bot commented Sep 14, 2023 • edited Loading

Codecov Report

lance6716 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 commented Sep 14, 2023

lance6716 commented Sep 14, 2023

lance6716 commented Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangenta left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 18, 2023

ti-chi-bot bot commented Sep 18, 2023

[LGTM Timeline notifier]

wjhuang2016 commented Sep 18, 2023

tiprow bot commented Sep 18, 2023

wjhuang2016 commented Sep 18, 2023

tiprow bot commented Sep 18, 2023

wjhuang2016 commented Sep 18, 2023

tiprow bot commented Sep 18, 2023

ti-chi-bot bot commented Sep 18, 2023

wjhuang2016 commented Sep 14, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading