Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ddl: make added count correct in adding index #6980

Merged
merged 2 commits into from
Jul 5, 2018
Merged

ddl: make added count correct in adding index #6980

merged 2 commits into from
Jul 5, 2018

Conversation

zimulala
Copy link
Contributor

@zimulala zimulala commented Jul 4, 2018 •
edited
Loading

What have you changed? (mandatory)

fix #6979
"RunInNewTxn" will do retry by itself when it encounters an error.

What are the type of the changes (mandatory)?

  • Improvement

@zimulala
Copy link
Contributor Author

zimulala commented Jul 4, 2018

PTAL @tiancaiamao @coocood @ciscoxll @crazycs520

@zimulala
Copy link
Contributor Author

zimulala commented Jul 4, 2018

/run-all-tests

jackysp
jackysp reviewed Jul 4, 2018
View reviewed changes
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 4, 2018
@crazycs520
Copy link
Contributor

If backfillIndexInTxn encounter error, "RunInNewTxn" do retry will also backfill the index which was already filled before encounter error, right?

@zimulala
Copy link
Contributor Author

zimulala commented Jul 4, 2018

/run-common-test

@zimulala
Copy link
Contributor Author

zimulala commented Jul 4, 2018

/run-integration-common-test

@zimulala
Copy link
Contributor Author

zimulala commented Jul 4, 2018

@crazycs520
In "backfillIndexInTxn" we only have the function of "RunInNewTxn". So if "backfillIndexInTxn" encounters an error, "RunInNewTxn" must encounter an error too. And if "RunInNewTxn" encounters an error, it will roll back by itself.

@zimulala
Copy link
Contributor Author

zimulala commented Jul 4, 2018

/run-common-test
/run-integration-common-test

crazycs520
crazycs520 reviewed Jul 4, 2018
View reviewed changes
ddl/index.go
errInTxn = kv.RunInNewTxn(w.sessCtx.GetStore(), true, func(txn kv.Transaction) error {
addedCount = 0
scanCount = 0
txn.SetOption(kv.Priority, kv.PriorityLow)
idxRecords, handleOutOfRange, err := w.fetchRowColVals(txn, handleRange)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indexRecord which was already filled before encounter error will also be backfilled again in retry process, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crazycs520
Yes, it will be backfilled again. If the RunInNewTxn meets an error, these code lines from 593 to 625 need to retry.

ciscoxll
ciscoxll reviewed Jul 5, 2018
View reviewed changes
Copy link
Contributor

@ciscoxll ciscoxll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ciscoxll ciscoxll added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 5, 2018
@shenli
Copy link
Member

shenli commented Jul 5, 2018

LGTM

@shenli
Copy link
Member

shenli commented Jul 5, 2018

/run-all-tests

@shenli shenli added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Jul 5, 2018
@coocood
Copy link
Member

coocood commented Jul 5, 2018

LGTM

1 similar comment
@crazycs520
Copy link
Contributor

LGTM

@jackysp
Copy link
Member

jackysp commented Jul 5, 2018

There are 5 LGTMs in this pr. But no one wants to approve it. :)

@shenli
Copy link
Member

shenli commented Jul 5, 2018

@jackysp I made the third LGTM, but I am waiting for the CI.

@ciscoxll ciscoxll merged commit bdfa5ee into pingcap:master Jul 5, 2018
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
@zimulala zimulala deleted the reorg-added-cnt branch August 4, 2022 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ciscoxll ciscoxll ciscoxll left review comments

@crazycs520 crazycs520 crazycs520 left review comments

@jackysp jackysp jackysp approved these changes

Assignees
No one assigned
Labels
sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make the value of "taskAddedCount" correct when adding index
7 participants
@zimulala @crazycs520 @shenli @coocood @jackysp @ciscoxll @you06