Skip to content

*: remove the LightningMode from TiDB#12466

Merged
sre-bot merged 6 commits intopingcap:masterfrom
lonng:remove-lightning-mode
Sep 29, 2019
Merged

*: remove the LightningMode from TiDB#12466
sre-bot merged 6 commits intopingcap:masterfrom
lonng:remove-lightning-mode

Conversation

@lonng
Copy link
Contributor

@lonng lonng commented Sep 28, 2019

Signed-off-by: Lonng heng@lonng.org

What problem does this PR solve?

The lightning does not use the TiDB's kvencoder anymore due to performance issue.

What is changed and how it works?

  1. Remove the lightning mode from TiDB.
  2. Remove the kvencoder from TiDB.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported variable/fields change

Release note

  • No need to add to release note.

Signed-off-by: Lonng <heng@lonng.org>
@lonng lonng added the type/enhancement The issue or PR belongs to an enhancement. label Sep 28, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 28, 2019

Thanks for your PR.
This PR will be closed by bot because you already had 3 opened PRs, close or merge them before submitting a new one.
#12322 , #12454 , #12460

@lonng
Copy link
Contributor Author

lonng commented Sep 28, 2019

@kennytm PTAL

@lonng lonng changed the title *: remove LightningMode from TiDB *: remove the LightningMode from TiDB Sep 28, 2019
@codecov
Copy link

codecov bot commented Sep 29, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #12466   +/-   ##
===========================================
  Coverage   79.9711%   79.9711%           
===========================================
  Files           462        462           
  Lines        103221     103221           
===========================================
  Hits          82547      82547           
  Misses        14768      14768           
  Partials       5906       5906

@ngaut
Copy link
Member

ngaut commented Sep 29, 2019

Nice clean up.

Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

The code is "rest LGTM" but I need to confirm if this will affect Lightning's import speed first (which is still using this flag).


func (s *session) getMembufCap() int {
if s.sessionVars.LightningMode {
return kv.ImportingTxnMembufCap
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the constant ImportingTxnMembufCap be removed as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch.

Signed-off-by: Lonng <heng@lonng.org>
@lonng
Copy link
Contributor Author

lonng commented Sep 29, 2019

@kennytm It's OK to remove the flag from TiDB-Lightning.

@lonng lonng reopened this Sep 29, 2019
@lonng lonng requested review from lysu and tiancaiamao September 29, 2019 04:42
@zyxbest
Copy link

zyxbest commented Sep 29, 2019

/run-unit-test

Copy link
Member

@ngaut ngaut left a comment

Choose a reason for hiding this comment

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

LGTM

@ngaut ngaut added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 29, 2019
@lonng lonng added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. status/PTAL labels Sep 29, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 29, 2019

/run-all-tests

@sre-bot sre-bot merged commit 41ac571 into pingcap:master Sep 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants