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
log-backup: added intervally resolve regions #14180
Conversation
Signed-off-by: hillium <yujuncen@pingcap.com>
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Signed-off-by: hillium <yujuncen@pingcap.com>
Signed-off-by: hillium <yujuncen@pingcap.com>
RegionCheckpointOperation::PrepareMinTsForResolve => { | ||
let min_ts = self.pool.block_on(self.prepare_min_ts()); | ||
let start_time = Instant::now(); | ||
try_send!( | ||
self.scheduler, | ||
Task::RegionCheckpointsOp(RegionCheckpointOperation::Resolve { | ||
min_ts, | ||
start_time | ||
}) | ||
); | ||
} | ||
RegionCheckpointOperation::Resolve { min_ts, start_time } => { | ||
let sched = self.scheduler.clone(); | ||
try_send!( | ||
self.scheduler, | ||
Task::ModifyObserve(ObserveOp::ResolveRegions { | ||
callback: Box::new(move |mut resolved| { | ||
let t = | ||
Task::RegionCheckpointsOp(RegionCheckpointOperation::Resolved { | ||
checkpoints: resolved.take_resolve_result(), | ||
start_time, | ||
}); | ||
try_send!(sched, t); | ||
}), | ||
min_ts | ||
}) | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that RegionCheckpointOperation::Resolve
is only scheduled by RegionCheckpointOperation::PrepareMinTsForResolve
?
RegionCheckpointOperation::PrepareMinTsForResolve => { | |
let min_ts = self.pool.block_on(self.prepare_min_ts()); | |
let start_time = Instant::now(); | |
try_send!( | |
self.scheduler, | |
Task::RegionCheckpointsOp(RegionCheckpointOperation::Resolve { | |
min_ts, | |
start_time | |
}) | |
); | |
} | |
RegionCheckpointOperation::Resolve { min_ts, start_time } => { | |
let sched = self.scheduler.clone(); | |
try_send!( | |
self.scheduler, | |
Task::ModifyObserve(ObserveOp::ResolveRegions { | |
callback: Box::new(move |mut resolved| { | |
let t = | |
Task::RegionCheckpointsOp(RegionCheckpointOperation::Resolved { | |
checkpoints: resolved.take_resolve_result(), | |
start_time, | |
}); | |
try_send!(sched, t); | |
}), | |
min_ts | |
}) | |
); | |
} | |
RegionCheckpointOperation::PrepareMinTsForResolve => { | |
let min_ts = self.pool.block_on(self.prepare_min_ts()); | |
let start_time = Instant::now(); | |
let sched = self.scheduler.clone(); | |
try_send!( | |
self.scheduler, | |
Task::ModifyObserve(ObserveOp::ResolveRegions { | |
callback: Box::new(move |mut resolved| { | |
let t = | |
Task::RegionCheckpointsOp(RegionCheckpointOperation::Resolved { | |
checkpoints: resolved.take_resolve_result(), | |
start_time, | |
}); | |
try_send!(sched, t); | |
}), | |
min_ts | |
}) | |
); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must reschedule the task to consume all the pending events after we updated the concurrency manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it
@Leavrth: 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: 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. |
Signed-off-by: hillium <yujuncen@pingcap.com>
…to add-resolved-ts
@@ -60,49 +64,59 @@ impl SubscriptionManager { | |||
while let Some(msg) = self.input.next().await { | |||
match msg { | |||
SubscriptionOp::Add(sub) => { | |||
self.subscribers.insert(Uuid::new_v4(), sub); | |||
let uid = Uuid::new_v4(); | |||
info!("log backup adding new subscriber"; "id" => %uid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will it cause too many logs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, one advancer in its lifetime should only subscribe once. 🤔
Signed-off-by: hillium <yujuncen@pingcap.com>
src/config/mod.rs
Outdated
@@ -2595,6 +2597,13 @@ impl BackupStreamConfig { | |||
); | |||
self.num_threads = default_cfg.num_threads; | |||
} | |||
if self.max_flush_interval < ReadableDuration::secs(1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe also check min_ts_interval
Signed-off-by: hillium <yujuncen@pingcap.com>
/test |
@YuJuncen: Your PR was out of date, I have automatically updated it for you. 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. |
/test |
(Perhaps we need to add a callback for |
/test |
/merge |
@hicqu: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger 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. |
This pull request has been accepted and is ready to merge. Commit hash: 96a0b08
|
In response to a cherrypick label: new pull request created to branch |
ref tikv#13638 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
In response to a cherrypick label: new pull request created to branch |
In response to a cherrypick label: new pull request created to branch |
ref tikv#13638 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ref tikv#13638 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
In response to a cherrypick label: new pull request created to branch |
ref tikv#13638 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
In response to a cherrypick label: new pull request created to branch |
ref tikv#13638 This PR added a “two phase” flush to log backup for reducing checkpoint lag. Generally, we added a `MinTs` task, where resolve the regions and advance the `resolved_ts` in the checkpoint manager. then, once we are doing flush, we would make current `resolved_ts` become `checkpoint_ts`. This allows us to advance checkpoint_ts even the leader has gone. When the leader changes frequently, this can greatly reduce checkpoint lag. Signed-off-by: hillium <yujuncen@pingcap.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>
ref #13638 This PR added a “two phase” flush to log backup for reducing checkpoint lag. Generally, we added a `MinTs` task, where resolve the regions and advance the `resolved_ts` in the checkpoint manager. then, once we are doing flush, we would make current `resolved_ts` become `checkpoint_ts`. This allows us to advance checkpoint_ts even the leader has gone. When the leader changes frequently, this can greatly reduce checkpoint lag. Signed-off-by: hillium <yujuncen@pingcap.com> Co-authored-by: 山岚 <36239017+YuJuncen@users.noreply.github.com> Co-authored-by: hillium <yujuncen@pingcap.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>
Signed-off-by: hillium yujuncen@pingcap.com
What is changed and how it works?
Issue Number: Ref #13638
What's Changed:
This PR is the "resolved TS" part of #14023.
Related changes
pingcap/docs
/pingcap/docs-cn
:Check List
Tests
Side effects
Release note