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

batchput set ttl to every key #10891

Merged
merged 8 commits into from Sep 23, 2021
Merged

batchput set ttl to every key #10891

merged 8 commits into from Sep 23, 2021

Conversation

daimashusheng
Copy link
Contributor

@daimashusheng daimashusheng commented Sep 2, 2021 •
edited by andylokandy

my issue: pingcap/kvproto#807

I add ttls param for BatchPut request so that it can set independent ttl for every key

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Sep 2, 2021 •
edited

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • andylokandy
  • iosmanthus

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added contribution Type: PR - From contributors size/M labels Sep 2, 2021
This was referenced Sep 3, 2021
Closed
Closed
Closed
@tisonkun
Copy link
Contributor

tisonkun commented Sep 6, 2021

@daimashusheng please elaborate the background of this PR or create an issue for it.

andylokandy
andylokandy reviewed Sep 14, 2021
View reviewed changes
components/test_storage/src/sync_storage.rs Outdated Show resolved Hide resolved
src/storage/mod.rs Outdated Show resolved Hide resolved
src/storage/mod.rs Outdated Show resolved Hide resolved
src/storage/txn/commands/atomic_store.rs Outdated Show resolved Hide resolved
src/storage/mod.rs Outdated Show resolved Hide resolved
src/server/service/kv.rs Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added size/L and removed size/M labels Sep 14, 2021
andylokandy
andylokandy reviewed Sep 14, 2021
View reviewed changes
components/error_code/src/sst_importer.rs Outdated Show resolved Hide resolved
components/error_code/src/storage.rs Outdated Show resolved Hide resolved
components/sst_importer/src/errors.rs Outdated Show resolved Hide resolved
src/server/service/kv.rs Outdated Show resolved Hide resolved
src/storage/mod.rs Outdated Show resolved Hide resolved
src/storage/mod.rs Show resolved Hide resolved
src/storage/mod.rs Outdated Show resolved Hide resolved
@andylokandy
Copy link
Contributor

Please sign off all of your commits or else the DCO check will complain.

@daimashusheng
1. add units test for batch_put ttls 2. using enable_ttl in atomic_st…
ce25867 
…ore instead of option<ttl>

Signed-off-by: tangjk <tangjiankun1226@gmail.com>
@daimashusheng
Copy link
Contributor Author

@daimashusheng seems there are no unit tests to cover the new interface, would you might add some?

added

@daimashusheng
just declare local varibale when it is used
160847f 
Signed-off-by: tangjk <tangjiankun1226@gmail.com>
@andylokandy
Copy link
Contributor

/run-all-tests

1 similar comment
@daimashusheng
Copy link
Contributor Author

/run-all-tests

@andylokandy
Copy link
Contributor

@iosmanthus PTAL

@iosmanthus
Copy link
Member

/run-all-tests

@iosmanthus
Copy link
Member

Rest LGTM. Waiting for the test result.

@daimashusheng
Copy link
Contributor Author

/run-all-tests

iosmanthus
iosmanthus approved these changes Sep 22, 2021
View reviewed changes
Copy link
Member

@iosmanthus iosmanthus left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot added status/LGT2 Status: PR - There are already 2 approvals and removed status/LGT1 Status: PR - There is already 1 approval labels Sep 22, 2021
@andylokandy
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

@andylokandy: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 160847f

@andylokandy
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

@andylokandy: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@andylokandy
Copy link
Contributor

/run-all-tests

5 similar comments
@daimashusheng
Copy link
Contributor Author

/run-all-tests

@daimashusheng
Copy link
Contributor Author

/run-all-tests

@daimashusheng
Copy link
Contributor Author

/run-all-tests

@daimashusheng
Copy link
Contributor Author

/run-all-tests

@daimashusheng
Copy link
Contributor Author

/run-all-tests

@ti-chi-bot ti-chi-bot merged commit f983827 into tikv:master Sep 23, 2021
v01dstar pushed a commit to v01dstar/tikv that referenced this pull request Oct 26, 2021
@daimashusheng @andylokandy @v01dstar
batchput set ttl to every key (tikv#10891)
e1ca1ed 
* rebase code to make other's commit history be eliminated

Signed-off-by: tangjk <tangjiankun1226@gmail.com>

* eliminate idx loop by zipping iter of ttls to kvs in atomic store

Signed-off-by: tangjk <tangjiankun1226@gmail.com>

* modify enum name make it's meaning clearer

Signed-off-by: tangjk <tangjiankun1226@gmail.com>

* remove nouseful code

Signed-off-by: tangjk <tangjiankun1226@gmail.com>

* make enum more readable

Signed-off-by: tangjk <tangjiankun1226@gmail.com>

* 1. add units test for batch_put ttls 2. using enable_ttl in atomic_store instead of option<ttl>

Signed-off-by: tangjk <tangjiankun1226@gmail.com>

* just declare local varibale when it is used

Signed-off-by: tangjk <tangjiankun1226@gmail.com>

Co-authored-by: Andy Lok <andylokandy@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andylokandy andylokandy andylokandy approved these changes

@iosmanthus iosmanthus iosmanthus approved these changes

Assignees
No one assigned
Labels
contribution Type: PR - From contributors first-time-contributor release-note-none size/L status/can-merge Status: Can merge to base branch status/LGT2 Status: PR - There are already 2 approvals
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@daimashusheng @ti-chi-bot @tisonkun @andylokandy @iosmanthus @marsishandsome @ti-srebot