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

log-backup: remove checkpoint V2 from codebase #13197

Merged
merged 14 commits into from Oct 11, 2022
Merged

log-backup: remove checkpoint V2 from codebase #13197

merged 14 commits into from Oct 11, 2022

Conversation

YuJuncen
Copy link
Contributor

@YuJuncen YuJuncen commented Aug 1, 2022 •
edited

Signed-off-by: Yu Juncen yujuncen@pingcap.com

What is changed and how it works?

Issue Number: Ref #13196

What's Changed:

This PR adapt the integration test cases for removing checkpoint v2.

Now, all integration test cases uses checkpoint V3.
Removed `test_inflight_message` because it is invalid in V3.

Check List

Tests

  • No code (Who tests tests?)

Release note 

None

@YuJuncen
adapt the integration test cases for checkpoint V3
9c42fe4 
Signed-off-by: Yu Juncen <yujuncen@pingcap.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 1, 2022 •
edited

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • 3pointer
  • overvenus

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 release-note-none contribution Type: PR - From contributors size/L labels Aug 1, 2022
@YuJuncen
run cargo fix
1b6c445 
Signed-off-by: Yu Juncen <yujuncen@pingcap.com>
@YuJuncen
fmt
5f3a694 
Signed-off-by: Yu Juncen <yujuncen@pingcap.com>
@YuJuncen
remove CheckpointV2Observer
481c1fe 
Signed-off-by: Yu Juncen <yujuncen@pingcap.com>
@ti-chi-bot ti-chi-bot added size/XL and removed size/L labels Aug 2, 2022
@YuJuncen
format
68f43d2 
Signed-off-by: Yu Juncen <yujuncen@pingcap.com>
@YuJuncen
fix build
3084545 
Signed-off-by: Yu Juncen <yujuncen@pingcap.com>
@YuJuncen
remove subscription tracker state
942b965 
Signed-off-by: Yu Juncen <yujuncen@pingcap.com>
@YuJuncen
Merge branch 'master' of https://github.com/tikv/tikv into adapt-v3-f…
47ebdb8 
…or-test

Signed-off-by: Yu Juncen <yujuncen@pingcap.com>
@YuJuncen YuJuncen changed the title log-backup: adapt the integration test cases for checkpoint V3 log-backup: remove checkpoint V2 from codebase Aug 4, 2022
@YuJuncen YuJuncen requested a review from 3pointer August 25, 2022 10:06
YuJuncen and others added 4 commits September 21, 2022 14:49
@YuJuncen
Merge branch 'master' into adapt-v3-for-test
576d11a
@YuJuncen
fixed lint
72f2ff7 
Signed-off-by: Yu Juncen <yujuncen@pingcap.com>
@YuJuncen
import pd_client
d0eb08f 
Signed-off-by: Yu Juncen <yujuncen@pingcap.com>
@YuJuncen
fix test cases
cc1e20b 
Signed-off-by: Yu Juncen <yujuncen@pingcap.com>
@YuJuncen
Copy link
Contributor Author

/run-tests

@joccau
Copy link
Contributor

joccau commented Sep 29, 2022

pub enum CheckpointProvider {
Store(u64),
Region { id: u64, version: u64 },
Task,
Global,
}
Can we remove the CheckpointProvider::Store and CheckpointProvider::Region.

tikv/components/backup-stream/src/metadata/client.rs

Line 76 in c48b0cf

pub enum CheckpointProvider {

overvenus
overvenus reviewed Oct 10, 2022
View reviewed changes
src/config.rs
@@ -2496,9 +2496,6 @@ pub struct BackupStreamConfig {
pub initial_scan_pending_memory_quota: ReadableSize,
#[online_config(skip)]
pub initial_scan_rate_limit: ReadableSize,
#[serde(skip)]
#[online_config(skip)]
pub use_checkpoint_v3: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Will it cause compatibility issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In all release version this field should be #[serde(skip)], I guess this would be fine.🤔

@ti-chi-bot ti-chi-bot added the status/LGT1 Status: PR - There is already 1 approval label Oct 10, 2022
joccau
joccau approved these changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@joccau joccau 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
Copy link
Member

@joccau: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM

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 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 Oct 11, 2022
@overvenus
Copy link
Member

/merge

@ti-chi-bot
Copy link
Member

@overvenus: 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: cc1e20b

@ti-chi-bot ti-chi-bot added the status/can-merge Status: Can merge to base branch label Oct 11, 2022
@YuJuncen
Copy link
Contributor Author

/run-test

@ti-chi-bot
Copy link
Member

@YuJuncen: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

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.

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.

@YuJuncen
Copy link
Contributor Author

/run-test

@YuJuncen
Copy link
Contributor Author

test_prevote_reboot_minority_followers 🤔
/run-test

@YuJuncen
Copy link
Contributor Author

test_read
/run-test

@YuJuncen
Copy link
Contributor Author

/run-test

1 similar comment
@YuJuncen
Copy link
Contributor Author

/run-test

@YuJuncen
Copy link
Contributor Author

operation::query::local::tests::test_read
/run-test

@ti-chi-bot ti-chi-bot merged commit be44dba into tikv:master Oct 11, 2022
@ti-chi-bot ti-chi-bot added this to the Pool milestone Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@overvenus overvenus overvenus approved these changes

@3pointer 3pointer 3pointer approved these changes

@joccau joccau joccau approved these changes

@hicqu hicqu Awaiting requested review from hicqu

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@YuJuncen @ti-chi-bot @joccau @overvenus @3pointer