*: refactor table.Allocator to improve readability #17238

tangenta · 2020-05-15T07:53:59Z

What problem does this PR solve?

Issue Number: N/A

What is changed and how it works?

Refactor the signature of method Alloc() in interface Allocator. The origin signature is:

Alloc(tableID int64, n uint64, increment, offset int64) (int64, int64, error)

The return values represent an interval that contains the first ID to allocate. It is neither easy to understand nor easy to use.
In this PR, I have changed it into:

Alloc(tableID int64, n uint64, increment, offset int64) (IDIterator, error)

The IDIterator represents a collection of allocated values. Here is the methods provided by IDIterator:

(iter *IDIterator) Next() (hasNext bool, result int64)
(iter *IDIterator) First() int64
(iter *IDIterator) Last() int64
(iter *IDIterator) Skip() *IDIterator
(iter *IDIterator) Count() uint64

To retrieve the values in it, one can keep calling Next() until the hasNext == false.

The implementations of signed and unsigned rebase/alloc are merged, so that code duplication is minimized.

Related changes

Check List

Tests

Unit test
Integration test

Side effects

Breaking backward compatibility

Release note

N/A

sre-bot · 2020-05-15T07:54:11Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

AilinKid · 2020-05-18T02:38:58Z

/bench

codecov · 2020-05-18T02:45:50Z

Codecov Report

Merging #17238 into master will decrease coverage by 0.4877%.
The diff coverage is 83.5106%.

@@               Coverage Diff                @@
##             master     #17238        +/-   ##
================================================
- Coverage   79.9304%   79.4427%   -0.4878%     
================================================
  Files           526        526                
  Lines        145584     142884      -2700     
================================================
- Hits         116366     113511      -2855     
- Misses        20019      20189       +170     
+ Partials       9199       9184        -15

sre-bot · 2020-05-18T05:25:47Z

Benchmark Report

Run Sysbench Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 2efab88a59499ea3934c8c1eba7b695cf24b9d29
+++ tidb: f78f7c687551542ac7127f826efb802831e9a1c5
tikv: e5afd630e35b42e970e6184e8b71eb98263a9363
pd: a80b99ef0d70807d106bdc1b7c6e97a118679c7e
================================================================================
oltp_update_index:
    * QPS: 4158.98 ± 2.54% (std=70.56) delta: -4.46% (p=0.073)
    * Latency p50: 30.79 ± 2.57% (std=0.53) delta: 4.61%
    * Latency p99: 56.33 ± 0.91% (std=0.51) delta: 1.78%
            
oltp_insert:
    * QPS: 7105.49 ± 0.36% (std=17.77) delta: 0.08% (p=0.715)
    * Latency p50: 18.01 ± 0.33% (std=0.04) delta: -0.08%
    * Latency p99: 30.81 ± 0.00% (std=0.00) delta: 0.00%
            
oltp_read_write:
    * QPS: 14600.60 ± 2.23% (std=193.65) delta: -1.77% (p=0.115)
    * Latency p50: 174.42 ± 0.46% (std=0.62) delta: 1.07%
    * Latency p99: 332.04 ± 3.63% (std=9.45) delta: 6.23%
            
oltp_point_select:
    * QPS: 41050.67 ± 2.62% (std=743.75) delta: -0.03% (p=0.981)
    * Latency p50: 3.12 ± 2.65% (std=0.06) delta: 0.08%
    * Latency p99: 10.46 ± 0.00% (std=0.00) delta: 0.00%

meta/autoid/autoid.go

AilinKid · 2020-05-18T10:36:13Z

meta/autoid/autoid.go

-	// Use uint64 n directly.
-	alloc.base = int64(uint64(alloc.base) + uint64(n1))
-	return min, alloc.base, nil
+	nr := (base + increment - offset) / increment


handle overflow here

AilinKid · 2020-05-18T11:08:52Z

meta/autoid/autoid.go

@@ -129,6 +128,49 @@ type Allocator interface {
 	GetType() AllocatorType
 }

+// IDIterator represent a list of values allocated by Alloc.alloc().
+type IDIterator struct {


maybe the computation in IDIterator should be under uint64.

refer the old code：id := int64(uint64(min) + uint64(j)*uint64(increment))

AilinKid · 2020-05-18T11:40:32Z

meta/autoid/autoid.go

+		if err != nil {
+			return IDIterator{}, err
+		}
+		alloc.lastAllocTime = time.Now()


better update alloc.step here like alloc.lastAllocTime does if it's not customized.
it's used to compute the next dynamic step.

if !alloc.customStep { alloc.step = nextStep }

alloc.adjustStep() updated the alloc.step:

func (alloc *allocator) adjustStep(startTime time.Time) { if alloc.customStep { return } consumeDur := startTime.Sub(alloc.lastAllocTime) alloc.step = NextStep(alloc.step, consumeDur) }

I mean, the step will also be modified in L541actualStep
Maybe we should record the actualStep to alloc.step in L549 if it's not user-customized.

if !alloc.customStep {
alloc.step = actualStep
}

AilinKid · 2020-06-19T06:57:43Z

reslove the conflicts, let's merge this

djshow832

The PR description says it needs to update the docs. Really? Why?

tangenta · 2020-06-30T16:37:34Z

The description is outdated.

This PR used to support auto_increment_increment on auto_random, but it is moved to another PR now.

meta/autoid/autoid.go

djshow832 · 2020-07-01T07:57:42Z

meta/autoid/autoid.go

+	// calcNeededBatchSize calculates the total batch size needed.
+	firstID, batchSize, expectedEnd, isOverflow := alloc.calcNeededBatchSize(alloc.base, int64(n), increment, offset)
+	if isOverflow {
+		return IDIterator{}, ErrAutoincReadFailed


Why this error?

It is consistent with MySQL.

djshow832 · 2020-10-29T07:02:08Z

executor/insert_common.go

 			// Assign autoIDs to rows.
 			for j := 0; j < cnt; j++ {
 				offset := j + start
 				d := rows[offset][colIdx]

-				id := int64(uint64(min) + uint64(j)*uint64(increment))
+				_, id := idIter.Next()


The first return value is ignored, it may be false.

wjhuang2016

I think this PR does too much compare to the PR description, it's better to split into some small PRs for review.

wjhuang2016 · 2020-11-03T07:10:22Z

meta/autoid/autoid.go

 	// It gets a batch of autoIDs at a time. So it does not need to access storage for each call.
 	// The consecutive feature is used to insert multiple rows in a statement.
-	// increment & offset is used to validate the start position (the allocator's base is not always the last allocated id).


Why do you remove these comments?

wjhuang2016 · 2020-11-03T07:11:25Z

meta/autoid/autoid.go

+}
+
+// Next returns the next allocated value.
+func (iter *IDIterator) Next() (bool, int64) {


I think it's better to return bool as the second result.

tangenta · 2020-11-06T02:52:57Z

I think this PR does too much compare to the PR description, it's better to split into some small PRs for review.

Ok.

zz-jason · 2021-02-09T08:31:12Z

I'm going to close this PR since it hasn't been updated for a long time. feel free to reopen if you want to continue with it. thank you for your contribution.

tangenta added 10 commits May 13, 2020 18:18

validate auto_increment_{increment|offset} session variables

5adf901

refactor Alloc() and Rebase() in allocator to reduce code duplication

596dce1

Enhance the usability of Allocator.Alloc()

585e606

make format

cb42200

improve one-by-one allocation performance

ee79d4f

improve readability of allocator.alloc()

2466252

remove unused code snippet

468c176

make auto_random aware auto_increment_{increment|offset}

51b7b0d

add a failpoint to skip increment and offset validation

ed010f3

add comments about IDIterator

8f0fe5c

tangenta added require-LGT3 Indicates that the PR requires three LGTM. sig/sql-infra SIG: SQL Infra labels May 15, 2020

tangenta requested a review from a team as a code owner May 15, 2020 07:54

ghost requested review from SunRunAway and removed request for a team May 15, 2020 07:54

tangenta requested review from AilinKid, tiancaiamao, bb7133 and a team May 15, 2020 07:54

ghost removed their request for review May 15, 2020 07:54

Merge branch 'master' into refactor-allocator

22f4411

github-actions bot added the sig/execution SIG execution label May 15, 2020

Merge branch 'master' into refactor-allocator

f78f7c6

clean up code

544dd2d

AilinKid reviewed May 18, 2020

View reviewed changes

Merge branch 'master' into refactor-allocator

d550ac6

AilinKid reviewed May 18, 2020

View reviewed changes

tangenta and others added 3 commits May 19, 2020 09:25

update comments

aabbdcf

Merge branch 'master' into refactor-allocator

b94569f

address comment

160f812

tangenta changed the title *: make auto_random aware of auto_increment_increment *: refactor table.Allocator to improve readability Jun 2, 2020

remove unrelated code

4465755

tiancaiamao removed their request for review June 9, 2020 06:00

AilinKid requested review from djshow832 and zimulala June 19, 2020 06:57

tangenta added 2 commits June 19, 2020 15:40

Merge remote-tracking branch 'upstream/master' into refactor-allocator

6b87b49

adjust return type order

134f514

djshow832 reviewed Jun 30, 2020

View reviewed changes

djshow832 reviewed Jul 1, 2020

View reviewed changes

github-actions bot added component/executor sig/executor labels Aug 5, 2020

AndreMouche removed the sig/executor label Aug 20, 2020

tangenta added 3 commits October 14, 2020 15:51

Merge remote-tracking branch 'upstream/master' into refactor-allocator

bba214d

address comments

d374505

make fmt

ebbc73c

djshow832 reviewed Oct 29, 2020

View reviewed changes

wjhuang2016 reviewed Nov 3, 2020

View reviewed changes

zz-jason closed this Feb 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: refactor table.Allocator to improve readability #17238

*: refactor table.Allocator to improve readability #17238

tangenta commented May 15, 2020 •

edited

Loading

sre-bot commented May 15, 2020

AilinKid commented May 18, 2020

codecov bot commented May 18, 2020 •

edited

Loading

sre-bot commented May 18, 2020

AilinKid May 18, 2020

AilinKid May 18, 2020

AilinKid May 18, 2020

tangenta May 19, 2020

AilinKid May 20, 2020 •

edited

Loading

AilinKid commented Jun 19, 2020

djshow832 left a comment

tangenta commented Jun 30, 2020

djshow832 Jul 1, 2020

tangenta Oct 29, 2020

djshow832 Oct 29, 2020

wjhuang2016 left a comment

wjhuang2016 Nov 3, 2020

wjhuang2016 Nov 3, 2020

tangenta commented Nov 6, 2020

zz-jason commented Feb 9, 2021

*: refactor table.Allocator to improve readability #17238

*: refactor table.Allocator to improve readability #17238

Conversation

tangenta commented May 15, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

sre-bot commented May 15, 2020

AilinKid commented May 18, 2020

codecov bot commented May 18, 2020 • edited Loading

Codecov Report

sre-bot commented May 18, 2020

Benchmark Report

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

AilinKid May 20, 2020 • edited Loading

Choose a reason for hiding this comment

AilinKid commented Jun 19, 2020

djshow832 left a comment

Choose a reason for hiding this comment

tangenta commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjhuang2016 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangenta commented Nov 6, 2020

zz-jason commented Feb 9, 2021

tangenta commented May 15, 2020 •

edited

Loading

codecov bot commented May 18, 2020 •

edited

Loading

AilinKid May 20, 2020 •

edited

Loading