-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
move region peer_cache to the peer cache itself #1859
Conversation
Any benchmark? |
LGTM |
I suggest moving peer_cache to another PR. |
OK, I will submit a new PR. |
please merge PR #1861 first. "replace hashmap raft.prs with flatmap" |
Please rebase master. |
7c2e543
to
a21be4d
Compare
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
PTAL @BusyJay |
@@ -192,14 +192,14 @@ pub struct PeerStat { | |||
pub struct Peer { | |||
engine: Arc<DB>, | |||
cfg: Rc<Config>, | |||
peer_cache: Rc<RefCell<HashMap<u64, metapb::Peer>>>, | |||
peer_cache: RefCell<FlatMap<u64, metapb::Peer>>, |
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.
If it's not going to be shared, then RefCell
is unnecessary.
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.
use RefCell for interior mutability, as get_peer_from_cache may insert new elem. e.g. store.validate_region
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.
I think it can be borrowed mutably.
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.
can we remove RefCell, @BusyJay ?
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.
Yes. But may not in this pr.
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.
@javaforfun please update it later.
LGTM |
…ikv#1859) * add low priority scheduler Signed-off-by: Ping Yu <yuping@pingcap.cn> * make low priority pool optional Signed-off-by: Ping Yu <yuping@pingcap.cn> --------- Signed-off-by: Ping Yu <yuping@pingcap.cn>
Fixes #1843.