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

raftstore: remove ts for bucket keys #12551

Merged
merged 16 commits into from May 18, 2022
Merged

Conversation

tonyxuqqi
Copy link
Contributor

Signed-off-by: qi.xu tonxuqi@outlook.com

What is changed and how it works?

Issue Number: Close #12549

What's Changed:

remove ts from bucket keys

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Release note

None

Signed-off-by: qi.xu <tonxuqi@outlook.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented May 17, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • 5kbpers
  • BusyJay

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.

.map(|k| ::keys::origin_key(&k).to_vec())
.collect(),
size: region_size,
.map(|k| strip_timestamp_if_exists(::keys::origin_key(&k).to_vec()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.map(|k| strip_timestamp_if_exists(::keys::origin_key(&k).to_vec()))
.map(|k| strip_timestamp_if_exists(::keys::origin_key(&k).to_vec())).dedup()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

你确定这个能编译吗?你上面dedup()的对象不是Vec<Vec>吧。

@ti-chi-bot ti-chi-bot added size/L and removed size/M labels May 17, 2022
qi.xu added 2 commits May 18, 2022 03:10
Signed-off-by: qi.xu <tonxuqi@outlook.com>
Signed-off-by: qi.xu <tonxuqi@outlook.com>
Copy link
Member

@BusyJay BusyJay left a comment

Choose a reason for hiding this comment

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

Can it be done by observer like split_obsever? For example, before buckets are taking effects, put them go through observers and strip timestamp.

@tonyxuqqi
Copy link
Contributor Author

Can it be done by observer like split_obsever? For example, before buckets are taking effects, put them go through observers and strip timestamp.

Probably not. There's no pre_propose like hook for buckets.

qi.xu added 2 commits May 18, 2022 04:54
Signed-off-by: qi.xu <tonxuqi@outlook.com>
@tonyxuqqi
Copy link
Contributor Author

/remove-label needs-cherry-pick-6.0

@tonyxuqqi
Copy link
Contributor Author

/assign @5kbpers

Signed-off-by: qi.xu <tonxuqi@outlook.com>
None
}
})
.coalesce(|prev, curr| {
Copy link
Member

Choose a reason for hiding this comment

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

This is almost the same as split check, why not reuse the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, the parameter for is_valid_split_key is different--here it's per bucket range, not per region range.

}
})
.collect::<Vec<_>>();
bucket.keys = adjusted_keys;
Copy link
Member

Choose a reason for hiding this comment

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

What if the key is beyond the region range?

Copy link
Contributor Author

@tonyxuqqi tonyxuqqi May 18, 2022

Choose a reason for hiding this comment

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

Theoretically it should not happen because bucket range is always within region range. But I can add an assert here.

qi.xu added 2 commits May 18, 2022 13:26
Signed-off-by: qi.xu <tonxuqi@outlook.com>
Signed-off-by: qi.xu <tonxuqi@outlook.com>
@tonyxuqqi
Copy link
Contributor Author

/remove-label needs-cherry-pick-6.0

@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 May 18, 2022
Signed-off-by: qi.xu <tonxuqi@outlook.com>
@tonyxuqqi
Copy link
Contributor Author

/re-test

@tonyxuqqi
Copy link
Contributor Author

/test

@tonyxuqqi
Copy link
Contributor Author

/remove-label need-cherry-pick-6.0

@ti-chi-bot
Copy link
Member

@tonyxuqqi: The label(s) need-cherry-pick-6.0 cannot be applied. These labels are supported: challenge-program, compatibility-breaker, high-performance, hptc, needs-cherry-pick-2.1, needs-cherry-pick-3.0, needs-cherry-pick-3.1, needs-cherry-pick-4.0, needs-cherry-pick-5.0, needs-cherry-pick-5.1, needs-cherry-pick-5.2, needs-cherry-pick-5.3, needs-cherry-pick-5.4, needs-cherry-pick-6.0, wontfix, do-not-merge/cherry-pick-not-approved, affects-4.0, affects-5.0, affects-5.1, affects-5.2, affects-5.3, affects-5.4, affects-6.0, may-affects-4.0, may-affects-5.0, may-affects-5.1, may-affects-5.2, may-affects-5.3, may-affects-5.4, may-affects-6.0.

In response to this:

/remove-label need-cherry-pick-6.0

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.

@tonyxuqqi
Copy link
Contributor Author

/remove-label needs-cherry-pick-6.0

@tonyxuqqi
Copy link
Contributor Author

/retest

@tonyxuqqi
Copy link
Contributor Author

/test

1 similar comment
@tonyxuqqi
Copy link
Contributor Author

/test

@tonyxuqqi
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

@tonyxuqqi: 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

@tonyxuqqi: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

In response to this:

/merge

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.

@5kbpers 5kbpers added the status/can-merge Status: Can merge to base branch label May 18, 2022
@ti-chi-bot ti-chi-bot removed the status/can-merge Status: Can merge to base branch label May 18, 2022
@5kbpers
Copy link
Member

5kbpers commented May 18, 2022

/merge

@ti-chi-bot
Copy link
Member

@5kbpers: 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: be9dc07

@ti-chi-bot ti-chi-bot added the status/can-merge Status: Can merge to base branch label May 18, 2022
@ti-chi-bot ti-chi-bot merged commit d5832c6 into tikv:master May 18, 2022
fengou1 pushed a commit to fengou1/tikv that referenced this pull request May 26, 2022
close tikv#12549

remove ts from bucket keys

Signed-off-by: qi.xu <tonxuqi@outlook.com>

Co-authored-by: qi.xu <tonxuqi@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

raftstore: bucket keys should not contain TS information
5 participants