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.

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

executor: add log for commit work #13965

Merged
merged 2 commits into from
Dec 9, 2019
Merged

Conversation

cfzjywxk
Copy link
Contributor

@cfzjywxk cfzjywxk commented Dec 8, 2019

What problem does this PR solve?

The original AddRecordLd function used in load data statement, will not report error to caller.

Problem:
Loading 300 millon rows in tpch lineitem table into tidb, after loaded row counts over 50800000, the load data commit goroutine will always write zero rows processing one commit task, because of error "[autoid:1467]Failed to read auto-increment value from storage engine" from AddRecord. (related to #13648, need more investigation on this), add this error not reported, so the loading process seems got stuck

What is changed and how it works?

  • Do report error in AddRecordLd function if AddRecord failed
  • Add some info log for commit processing

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below), retrying loading tpch.lineitem data with 300million rows

Code changes

Side effects

Related changes

  • Need to cherry-pick to the release branch ?

Release note

@cfzjywxk cfzjywxk added type/bugfix This PR fixes a bug. sig/execution SIG execution labels Dec 8, 2019
@codecov
Copy link

codecov bot commented Dec 8, 2019

Codecov Report

Merging #13965 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #13965   +/-   ##
===========================================
  Coverage   80.1928%   80.1928%           
===========================================
  Files           480        480           
  Lines        120416     120416           
===========================================
  Hits          96565      96565           
  Misses        16158      16158           
  Partials       7693       7693

@jackysp jackysp changed the title [executor ]add log for commit work executor: add log for commit work Dec 9, 2019
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

Copy link

@imtbkcat imtbkcat left a comment

Choose a reason for hiding this comment

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

LGTM

@imtbkcat
Copy link

imtbkcat commented Dec 9, 2019

Does this pr need to be cherry-pick to release-3.0? @cfzjywxk

@imtbkcat
Copy link

imtbkcat commented Dec 9, 2019

/run-all-tests

@cfzjywxk
Copy link
Contributor Author

cfzjywxk commented Dec 9, 2019

Does this pr need to be cherry-pick to release-3.0?

@jackysp @imtbkcat I think so, seems 3.0 has the same problem

@cfzjywxk
Copy link
Contributor Author

cfzjywxk commented Dec 9, 2019

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 9, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 9, 2019

/run-all-tests

@sre-bot sre-bot merged commit c503553 into pingcap:master Dec 9, 2019
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants