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

GC V2: Support different gc_safepoint for different keyspace #7684

Merged
merged 27 commits into from
Jun 27, 2023
Merged

GC V2: Support different gc_safepoint for different keyspace #7684

merged 27 commits into from
Jun 27, 2023

Conversation

ystaticy
Copy link
Contributor

@ystaticy ystaticy commented Jun 20, 2023 •
edited
Loading

What problem does this PR solve?

Issue Number: ref #7697

Problem Summary:

What is changed and how it works?

Check List

Tests

  • Manual test (add detailed scripts or steps below)

Release note

None

@ystaticy
support keyspace gc
bcc49d5 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 20, 2023
@sre-bot
Copy link
Collaborator

sre-bot commented Jun 20, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@ti-chi-bot ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 20, 2023
ystaticy added 14 commits June 20, 2023 15:53
@ystaticy
support keyspace gc
d6110c3 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
support keyspace gc
52437c9 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
fix use clock
e1b1969 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
support keyspace gc
83b7f20 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
fix use clock
2d398ee 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
fix use clock
bb850e2 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
fix use clock
d010278 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
code format
a3a20eb 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
code format
4ee7284 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
code format
c8f0adf 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
code format
93b7c93 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
Merge branch 'master' into yaojingyi/ks_gc
bbc765b
@ystaticy
add debug log
217ec94 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
code format
5601888 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy ystaticy marked this pull request as ready for review June 26, 2023 05:44
@ti-chi-bot ti-chi-bot bot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-linked-issue labels Jun 26, 2023
@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jun 26, 2023
hongyunyan
hongyunyan reviewed Jun 27, 2023
View reviewed changes
dbms/src/Server/Server.cpp Outdated Show resolved Hide resolved
dbms/src/Storages/Transaction/PDTiKVClient.h Outdated Show resolved Hide resolved
dbms/src/Storages/Transaction/PDTiKVClient.h Outdated Show resolved Hide resolved
ystaticy added 2 commits June 27, 2023 11:01
@ystaticy
remove unused code
19f14d5 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
fix comments
79047a2 
Signed-off-by: ystaticy <y_static_y@sina.com>
ystaticy added 2 commits June 27, 2023 14:39
@ystaticy
fix comments
6e36f9c 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
fix comments
e22ad7e 
Signed-off-by: ystaticy <y_static_y@sina.com>
hongyunyan
hongyunyan approved these changes Jun 27, 2023
View reviewed changes
Copy link
Contributor

@hongyunyan hongyunyan left a comment

Choose a reason for hiding this comment

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

rest is LGTM

dbms/src/Server/Server.cpp Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 27, 2023
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jun 27, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-06-26 08:57:16.898575867 +0000 UTC m=+611602.299826300: ☑️ agreed by breezewish.
  • 2023-06-27 07:12:51.251607695 +0000 UTC m=+691736.652858495: ☑️ agreed by hongyunyan.

ystaticy added 5 commits June 27, 2023 15:17
@ystaticy
fix comments
77fd344 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
fix comments
252cff2 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
fix comments
a8bfbd0 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
fix comments
79bd6ac 
Signed-off-by: ystaticy <y_static_y@sina.com>
@ystaticy
fix comments
aa2850d 
Signed-off-by: ystaticy <y_static_y@sina.com>
@JaySon-Huang JaySon-Huang changed the title support keyspace gc Support different gc_safepoint for different keyspace Jun 27, 2023
JaySon-Huang
JaySon-Huang approved these changes Jun 27, 2023
View reviewed changes
Copy link
Contributor

@JaySon-Huang JaySon-Huang left a comment

Choose a reason for hiding this comment

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

LGTM with a logging style comment

dbms/src/Storages/Transaction/PDTiKVClient.h
if (enable_safepoint_v2 && keyspace_id != NullspaceID)
{
auto gc_safe_point = getGCSafePointV2WithRetry(pd_client, keyspace_id, ignore_cache, safe_point_update_interval_seconds);
LOG_DEBUG(Logger::get(), "use safe point v2, keyspace_id={},gc_safe_point={}", keyspace_id, gc_safe_point);
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
LOG_DEBUG(Logger::get(), "use safe point v2, keyspace_id={},gc_safe_point={}", keyspace_id, gc_safe_point);
LOG_DEBUG(Logger::get(), "use safe point v2, keyspace={} gc_safe_point={}", keyspace_id, gc_safe_point);

Other codes use 'keyspace=xxx' in logging, so this is better for searching logs

tiflash/dbms/src/Storages/GCManager.cpp

Line 69 in 4ecf3cd

LOG_DEBUG(log, "Start GC with keyspace={}, table_id={}", next_keyspace_table_id.first, next_keyspace_table_id.second);

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jun 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: breezewish, hongyunyan, JaySon-Huang

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [JaySon-Huang,breezewish,hongyunyan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hongyunyan
Copy link
Contributor

/run-all-tests

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jun 27, 2023

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

trigger some heavy tests which will not run always when PR updated.

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.

@ti-chi-bot ti-chi-bot bot merged commit 11179b9 into pingcap:master Jun 27, 2023
@ystaticy ystaticy mentioned this pull request Dec 6, 2023
Open
23 tasks
@ystaticy ystaticy changed the title Support different gc_safepoint for different keyspace GC V2: Support different gc_safepoint for different keyspace Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hongyunyan hongyunyan hongyunyan approved these changes

@JaySon-Huang JaySon-Huang JaySon-Huang approved these changes

@breezewish breezewish breezewish approved these changes

Assignees
No one assigned
Labels
approved lgtm release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ystaticy @sre-bot @hongyunyan @breezewish @JaySon-Huang