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
config: support online change lock manager config #6338
Conversation
Signed-off-by: linning <linningde25@gmail.com>
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.
Rest LGTM
// Create CoprocessorHost. | ||
let mut coprocessor_host = | ||
CoprocessorHost::new(self.config.coprocessor.clone(), self.router.clone()); | ||
|
||
let lock_mgr = if self.config.pessimistic_txn.enabled { | ||
let lock_mgr = LockManager::new(); | ||
cfg_controller.register("pessimistic_txn", Box::new(lock_mgr.config_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.
pessimistic_txn
or pessimistic-txn
?
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.
pessimistic_txn
same as the TiKvConfig
field name.
change.remove("wait_for_lock_timeout").map(Into::into), | ||
change.remove("wake_up_delay_duration").map(Into::into), | ||
) { | ||
(timeout @ Some(_), delay) => { |
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.
What if delay
is None?
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.
WaiterManager
need to listening both update of timeout
and delay
, even one of them are not change (represent as None
here), we still need to dispatch the change to it, and WaiterManager::handle_config_change
will take care of various situations.
@@ -319,6 +329,9 @@ impl Display for Task { | |||
), | |||
Task::DetectRpc { .. } => write!(f, "Detect Rpc"), | |||
Task::ChangeRole(role) => write!(f, "ChangeRole {{ role: {:?} }}", role), | |||
Task::ChangeTTL(ttl) => write!(f, "ChangeTTL {{ ttl: {:?} }}", ttl), |
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.
Task::ChangeTTL(ttl) => write!(f, "ChangeTTL {{ ttl: {:?} }}", ttl), | |
Task::ChangeTTL(ttl) => write!(f, "ChangeTTL { ttl: {:?} }", ttl), |
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 need {{
here to print out {
, so Task::ChangeTTL
print something like ChangeTTL { ttl: 100ms }
.
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.
Rest LGTM
Signed-off-by: linning <linningde25@gmail.com>
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.
LGTM
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.
LGTM
/run-all-tests |
@NingLin-P merge failed. |
/merge |
/run-all-tests |
Signed-off-by: linning <linningde25@gmail.com>
What have you changed?
This pr is part of online change config.
This pr add support for online change lock manager config by adding new tasks to lock manger (
Task::ChangeConfig
andTask::ChangeTTL
).What is the type of the changes?
How is the PR tested?
Does this PR affect documentation (docs) or should it be mentioned in the release notes?
No
Does this PR affect
tidb-ansible
?No