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.

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

raftstore: remove message box/unbox #4232

Merged
merged 17 commits into from Mar 5, 2019

Conversation

Projects
None yet
5 participants
@BusyJay
Copy link
Contributor

BusyJay commented Feb 19, 2019

What have you changed? (mandatory)

This PR removes the redundant enum Msg introduced in #4066, which aimed to maintain a compatible layer and made refactor more smoothly. Now that the PR is merged, it's time to clean up. This PR also simplifies the signature of simulate transport by merge SendFilter and RecvFilter as a unify Filter.

What are the type of the changes? (mandatory)

  • Improvement

How has this PR been tested? (mandatory)

unit tests

Does this PR affect documentation (docs) update? (mandatory)

No.

Does this PR affect tidb-ansible update? (mandatory)

No.

Refer to a related PR or issue link (optional)

#4066

raftstore: remove message box/unbox
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
Merge branch 'master' into unbox-message
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
@BusyJay

This comment has been minimized.

Copy link
Contributor Author

BusyJay commented Feb 20, 2019

PTAL

Show resolved Hide resolved src/pd/pd.rs Outdated
@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Feb 20, 2019

any performance increased?

@BusyJay

This comment has been minimized.

Copy link
Contributor Author

BusyJay commented Feb 21, 2019

No notable improvement. Box/unbox is pretty cheap comparing all other costs.

address comment
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
@BusyJay

This comment has been minimized.

Copy link
Contributor Author

BusyJay commented Feb 25, 2019

PTAL

Merge branch 'master' into unbox-message
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
})) {
if let Err(e) = router.send(
source_region_id,
PeerMsg::CasualMessage(CasualMessage::MergeResult {

This comment has been minimized.

@Connor1996

Connor1996 Feb 27, 2019

Member

MergeResult is not casual. For the merge isolation recovery mechanism, it relies on MergeResult to destroy overlapping source peer.

This comment has been minimized.

@BusyJay

BusyJay Feb 27, 2019

Author Contributor

If it's lost, it will be resent again.

This comment has been minimized.

@Connor1996

Connor1996 Mar 4, 2019

Member

where to resent?

This comment has been minimized.

@Connor1996

Connor1996 Mar 4, 2019

Member

seems only resent in try_send, but it is still possible that MergeResult fails to send due to channel full.

Show resolved Hide resolved components/test_raftstore/src/cluster.rs Outdated
Show resolved Hide resolved components/test_raftstore/src/server.rs Outdated
Show resolved Hide resolved components/test_raftstore/src/server.rs Outdated
#[derive(Debug, PartialEq, Clone, Copy)]
pub enum DiscardReason {
Disconnected,
Filtered,

This comment has been minimized.

@overvenus

overvenus Feb 27, 2019

Member

Where do you use Filtered?

This comment has been minimized.

@BusyJay

BusyJay Feb 28, 2019

Author Contributor

In tests.

This comment has been minimized.

@overvenus

overvenus Feb 28, 2019

Member

What about #[cfg(test)]?

This comment has been minimized.

@BusyJay

BusyJay Feb 28, 2019

Author Contributor

No, integration tests links to the library with test profile disabled.

This comment has been minimized.

@overvenus

overvenus Mar 1, 2019

Member

Could you add some comments?


/// Routes message to target region.
///
/// This trait should only be used for internal commands. And messages

This comment has been minimized.

@overvenus

overvenus Feb 27, 2019

Member

It feels ambiguous, what is the inernal? Can it be pub (super) or other visibility?

}

/// Routes proposal to target region.
pub trait ProposalRouter {

This comment has been minimized.

@overvenus

overvenus Feb 27, 2019

Member

What about type alias instead of traits?

This comment has been minimized.

@BusyJay

BusyJay Feb 28, 2019

Author Contributor

Using traits to make it mock-able.

address comment
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
Show resolved Hide resolved components/test_raftstore/src/cluster.rs
Show resolved Hide resolved components/test_raftstore/src/node.rs
Show resolved Hide resolved components/test_raftstore/src/transport_simulate.rs
Show resolved Hide resolved src/raftstore/coprocessor/split_check/size.rs Outdated
Show resolved Hide resolved src/raftstore/coprocessor/split_check/size.rs Outdated
Show resolved Hide resolved src/raftstore/store/fsm/peer.rs
Show resolved Hide resolved src/raftstore/store/msg.rs Outdated
Show resolved Hide resolved src/raftstore/store/msg.rs Outdated
///
/// This trait should only be used for internal commands. And messages
/// are not guranteed to be delivered by this trait.
pub trait StoreRouter {
Show resolved Hide resolved src/raftstore/store/worker/read.rs Outdated

BusyJay added some commits Feb 28, 2019

address comment
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
address comment
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
Merge branch 'master' into unbox-message
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
@BusyJay

This comment has been minimized.

Copy link
Contributor Author

BusyJay commented Feb 28, 2019

PTAL

@@ -26,8 +26,7 @@ pub trait Transport: Send + Clone {

/// Routes message to target region.
///
/// This trait should only be used for internal commands. And messages
/// are not guranteed to be delivered by this trait.
/// Messages are not guranteed to be delivered by this trait.

This comment has been minimized.

@Hoverbear

Hoverbear Feb 28, 2019

Member

guranteed -> guaranteed

Hoverbear and others added some commits Feb 28, 2019

address comment
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
address comment
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
Merge branch 'master' into unbox-message
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
@BusyJay

This comment has been minimized.

Copy link
Contributor Author

BusyJay commented Mar 4, 2019

/run-integration-tests

@Connor1996
Copy link
Member

Connor1996 left a comment

rest LGTM

@@ -482,7 +482,7 @@ fn dummpy_filter(_: &CompactionJobInfo) -> bool {

pub fn create_test_engine(
engines: Option<Engines>,
tx: SendCh,
tx: RaftRouter,

This comment has been minimized.

@Connor1996

Connor1996 Mar 4, 2019

Member
Suggested change
tx: RaftRouter,
router: RaftRouter,
@@ -92,15 +91,15 @@ pub struct KeysCheckObserver<C> {
region_max_keys: u64,
split_keys: u64,
batch_split_limit: u64,
ch: Mutex<RetryableSendCh<Msg, C>>,
ch: Mutex<C>,

This comment has been minimized.

@Connor1996

Connor1996 Mar 4, 2019

Member
Suggested change
ch: Mutex<C>,
router: Mutex<C>,
@@ -1136,13 +1135,13 @@ fn notify_stats(ch: Option<&SendCh>) {
pub struct SnapManager {
// directory to store snapfile.
core: Arc<RwLock<SnapManagerCore>>,
ch: Option<SendCh>,
ch: Option<RaftRouter>,

This comment has been minimized.

@Connor1996

Connor1996 Mar 4, 2019

Member
Suggested change
ch: Option<RaftRouter>,
router: Option<RaftRouter>,
limiter: Option<Arc<IOLimiter>>,
max_total_size: u64,
}

impl SnapManager {
pub fn new<T: Into<String>>(path: T, ch: Option<SendCh>) -> SnapManager {
pub fn new<T: Into<String>>(path: T, ch: Option<RaftRouter>) -> SnapManager {

This comment has been minimized.

@Connor1996

Connor1996 Mar 4, 2019

Member
Suggested change
pub fn new<T: Into<String>>(path: T, ch: Option<RaftRouter>) -> SnapManager {
pub fn new<T: Into<String>>(path: T, router: Option<RaftRouter>) -> SnapManager {
pub fn new(
store_id: u64,
store_ch: SendCh,
store_ch: S,

This comment has been minimized.

@Connor1996

Connor1996 Mar 4, 2019

Member
Suggested change
store_ch: S,
store_router: S,
});
if let Err(e) = self.ch.try_send(msg) {
};
if let Err(e) = self.ch.send(region_id, msg) {

This comment has been minimized.

@Connor1996

Connor1996 Mar 4, 2019

Member
Suggested change
if let Err(e) = self.ch.send(region_id, msg) {
if let Err(e) = self.router.send(region_id, msg) {
@@ -184,35 +182,26 @@ impl Task {
Task::Destroy(region_id)
}

pub fn read(msg: StoreMsg) -> Task {

This comment has been minimized.

@Connor1996

Connor1996 Mar 4, 2019

Member

why remove this

This comment has been minimized.

@BusyJay

BusyJay Mar 4, 2019

Author Contributor

Task::Read is enough.

This comment has been minimized.

@Connor1996

Connor1996 Mar 4, 2019

Member

Register,Update,Destroy seems unnecessary too

This comment has been minimized.

@BusyJay

BusyJay Mar 4, 2019

Author Contributor

There are extra steps in register and update. destroy is redundant though. @overvenus what do you think?

This comment has been minimized.

@overvenus

overvenus Mar 5, 2019

Member

Better to keep consistent APIs, so I think we should either keep the read or remove others.

debug!("localreader redirects command"; "tag" => &self.tag, "command" => ?cmd);
let region_id = cmd.request.get_header().get_region_id();
let mut err = errorpb::Error::new();
match self.ch.send(cmd) {

This comment has been minimized.

@Connor1996

Connor1996 Mar 4, 2019

Member
Suggested change
match self.ch.send(cmd) {
match self.router.send(cmd) {
engine: Arc<DB>,
ch: RetryableSendCh<Msg, C>,
ch: S,

This comment has been minimized.

@Connor1996

Connor1996 Mar 4, 2019

Member
Suggested change
ch: S,
router: S,

BusyJay added some commits Mar 4, 2019

address comment
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
address comment
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
@BusyJay

This comment has been minimized.

Copy link
Contributor Author

BusyJay commented Mar 5, 2019

PTAL

@Connor1996
Copy link
Member

Connor1996 left a comment

LGTM

@BusyJay

This comment has been minimized.

Copy link
Contributor Author

BusyJay commented Mar 5, 2019

/run-integration-tests

1 similar comment
@BusyJay

This comment has been minimized.

Copy link
Contributor Author

BusyJay commented Mar 5, 2019

/run-integration-tests

@BusyJay BusyJay merged commit e462c0a into tikv:master Mar 5, 2019

5 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

@BusyJay BusyJay deleted the BusyJay:unbox-message branch Mar 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.