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/config: support merge #2819

Merged
merged 4 commits into from
Mar 14, 2018
Merged

Conversation

BusyJay
Copy link
Member

@BusyJay BusyJay commented Mar 13, 2018

This pr adds configuration entries for merge. Actual implementation see #2502.

@@ -435,6 +437,7 @@ fn test_snapshot_with_append<T: Simulator>(cluster: &mut Cluster<T>) {
// truncate the log quickly so that we can force sending snapshot.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about add a method with_test_raftstore_config for Cluster?

@@ -149,6 +154,8 @@ impl Default for Config {
raft_store_max_leader_lease: ReadableDuration::secs(9),
right_derive_when_split: true,
allow_remove_leader: false,
max_merge_log_gap: 10,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not merge_max_log_gap?

@hicqu
Copy link
Contributor

hicqu commented Mar 13, 2018

rest LGTM.

Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

LGTM

@BusyJay
Copy link
Member Author

BusyJay commented Mar 13, 2018

/run-integration-tests

@BusyJay
Copy link
Member Author

BusyJay commented Mar 13, 2018

PTAL

@@ -95,6 +95,11 @@ pub struct Config {

pub allow_remove_leader: bool,

/// Max log gap allowed to propose merge.
pub merge_max_log_gap: u64,
// Interval to repropose merge.
Copy link
Member

Choose a reason for hiding this comment

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

Please use ///

@BusyJay
Copy link
Member Author

BusyJay commented Mar 14, 2018

PTAL

@BusyJay
Copy link
Member Author

BusyJay commented Mar 14, 2018

/run-integration-tests

Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

LGTM

@siddontang
Copy link
Contributor

CI failed @BusyJay

@BusyJay
Copy link
Member Author

BusyJay commented Mar 14, 2018

/run-integration-common-test

@BusyJay BusyJay merged commit 6f99309 into tikv:master Mar 14, 2018
@BusyJay BusyJay deleted the add-merge-configuration branch March 14, 2018 06:15
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants