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

store/tikv: Make maxTxnTimeUse a configurable value #8215

Merged

Conversation

MyonKeminta
Copy link
Contributor

Signed-off-by: MyonKeminta MyonKeminta@users.noreply.github.com

What problem does this PR solve?

Sometimes in an olap senario, an insert into select statement may take too long time to finish. Then the maxTxnTimeUse (590 seconds) may easily be exceeded. It's better to make it configurable.

The maxTxnTimeUse is required because we don't want the transaction be broken by GC. That's why we recommend users not to set the gc_life_time less than 10m. However we didn't really check if the gc_life_time was set too short. In this PR, I still didn't check it.

What is changed and how it works?

This PR adds a field max-txn-time-use to the config file, tikv-client` section. Then in 2pc.go use the config value instead of a constant.

Check List

Tests

  • Unit test

Code changes

  • None of those listed

Side effects

  • None of those listed

Related changes

  • Need to update the documentation
  • Need to be included in the release note

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@MyonKeminta
Copy link
Contributor Author

@disksing
Copy link
Contributor

disksing commented Nov 7, 2018

LGTM

@@ -15,6 +15,7 @@ package tikv

import (
"bytes"
"github.com/pingcap/tidb/config"
Copy link
Member

Choose a reason for hiding this comment

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

put this package into the package block of the third-part packages

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@zz-jason zz-jason added type/enhancement The issue or PR belongs to an enhancement. status/LGT1 Indicates that a PR has LGTM 1. component/tikv labels Nov 7, 2018
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 7, 2018
@zz-jason
Copy link
Member

zz-jason commented Nov 7, 2018

/run-all-tests

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.

If we set it in the config file, e.g. 1 hour, when the gc life time change to 10 min. What will happen?
I think we'd better save the gc life time in pd like ddl schema version.

@jackysp
Copy link
Member

jackysp commented Nov 7, 2018

PTAL @tiancaiamao

@MyonKeminta
Copy link
Contributor Author

@jackysp In this PR it's user's responsibility to ensure maxTxnTime is less than gc_life_time. There may be a better solution, but it will take some time to get it done.

@MyonKeminta
Copy link
Contributor Author

CI failed.. I'm not sure if that's caused by my chagnes

@zz-jason
Copy link
Member

zz-jason commented Nov 8, 2018

/run-integration-ddl-test

@MyonKeminta
Copy link
Contributor Author

@tiancaiamao PTAL

@MyonKeminta
Copy link
Contributor Author

May this PR merge? @jackysp

@jackysp jackysp removed the status/DNM label Nov 9, 2018
@jackysp
Copy link
Member

jackysp commented Nov 9, 2018

Hold on. PTAL @tiancaiamao .

@MyonKeminta
Copy link
Contributor Author

@tiancaiamao

@tiancaiamao
Copy link
Contributor

LGTM
This is a risky configure option, but I haven't find better way to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/tikv 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