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

txn: Add txn_heart_beat API #5407

Merged
merged 8 commits into from Sep 6, 2019

Conversation

@MyonKeminta
Copy link
Contributor

commented Sep 5, 2019

What have you changed?

Please explain in detail what the changes are in this PR and why they are needed:

This PR adds txn_heart_beat API to TiKV. This feature is required for supporting Large Transaction.

txn_heart_beat API should be invoked on primary lock on a transaction and try to enlarge the lock's TTL to the specified value.

What is the type of the changes?

  • New feature (a change which adds functionality)

How is the PR tested?

By unit tests. It's better that we do some further tests.

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

No.

Does this PR affect tidb-ansible?

No.

Refer to a related PR or issue link (optional)

Corresponding PR in TiDB: pingcap/tidb#11979

MyonKeminta added 2 commits Sep 5, 2019
Implement txn_heart_beat
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Add tests for txn_heart_beat
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

/run-integration-common-test

@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

/run-integration-common-test

Add metrics
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

LGTM

@coocood

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Need to update gen_command_lock?

@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@coocood Yes you are right. I forgot it again. We should avoid using wildcards _ in this kind of code to avoid forgotting adding new invariants to these matches.

Address comments
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@coocood

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

LGTM

@AndreMouche

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

@MyonKeminta MyonKeminta requested review from youjiali1995 and sticnarf Sep 6, 2019

if lock.ttl < advise_ttl {
lock.ttl = advise_ttl;
self.put_lock(primary_key, &lock);
MVCC_TXN_HEART_BEAT_UPDATE_TTL.inc();

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Sep 6, 2019

Contributor

I think it's similar to grpc_msg_count.

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Sep 6, 2019

Author Contributor

Here I want to count only the requests that do update the lock's ttl. grcp_msg_count sounds like includes requests that didn't update it.

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Sep 6, 2019

Contributor

Yes. I think almost every request will update ttl...

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Sep 6, 2019

Author Contributor

Oh, that make sense. So should I just remove the metric or add a metric for requests that didn't update ttl?

This comment has been minimized.

Copy link
@sticnarf

sticnarf Sep 6, 2019

Contributor

What about logging the abnormality if advise_ttl < lock.ttl?

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Sep 6, 2019

Member

I think we can leave some debug log for the case device_ttl<lock.ttl.

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Sep 6, 2019

Author Contributor

Ok.

@youjiali1995

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

@coocood Yes you are right. I forgot it again. We should avoid using wildcards _ in this kind of code to avoid forgotting adding new invariants to these matches.

#4614 doesn't help?

@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

It helps and panicked in TiDB's integration test.
I just noticed that I forgot to add tests for the storage. I'll add it. Wait a minute.

Add test in storage.mod
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

@youjiali1995 PTAL again thanks

)
.unwrap();
rx.recv().unwrap();

This comment has been minimized.

Copy link
@sticnarf

sticnarf Sep 6, 2019

Contributor

Should we also get the lock directly and check its TTL?

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Sep 6, 2019

Author Contributor

The function to get lock which is scan_lock, doesn't get the TTL. So I didn't check it directly.

Context::default(),
Key::from_raw(b"k"),
10,
90,

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Sep 6, 2019

Member

how about define
advice_ttl=90; expect_ttl=100
to make the test case more easily to understand?

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Sep 6, 2019

Author Contributor

I think it may be still difficult to read since you need to find the variable's definition or assignment to know what value it is. I think I'll add comments near these arguments.

storage
.async_txn_heart_beat(
Context::default(),
Key::from_raw(b"k"),

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Sep 6, 2019

Member

How about defining a constant key so we can easily find we are always processing the same key?

if lock.ttl < advise_ttl {
lock.ttl = advise_ttl;
self.put_lock(primary_key, &lock);
MVCC_TXN_HEART_BEAT_UPDATE_TTL.inc();

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Sep 6, 2019

Member

I think we can leave some debug log for the case device_ttl<lock.ttl.

address comments
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
10,
100,
k.clone(),
/*start_ts*/ 10,

This comment has been minimized.

Copy link
@sticnarf

sticnarf Sep 6, 2019

Contributor

I remember block comments are not idiomatic in Rust...

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Sep 6, 2019

Author Contributor

I think I can add a description on each invoke of the function instead commenting arguments..

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

left a comment

LGTM

@AndreMouche
Copy link
Member

left a comment

LGTM

@MyonKeminta MyonKeminta referenced this pull request Sep 6, 2019
5 of 5 tasks complete

@sticnarf sticnarf added the S: CanMerge label Sep 6, 2019

@sre-bot

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2019

/run-all-tests

@sre-bot sre-bot merged commit 369e400 into tikv:master Sep 6, 2019

6 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-cop-push-down-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
@sre-bot

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2019

cherry pick to release-3.0 failed

NingLin-P added a commit to NingLin-P/tikv that referenced this pull request Sep 9, 2019
txn: Add txn_heart_beat API (tikv#5407)
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

@MyonKeminta MyonKeminta deleted the MyonKeminta:misono/txn-heart-beat branch Sep 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.