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: increase region size from 64M to 96M #1449

Merged
merged 33 commits into from
May 24, 2017

Conversation

ngaut
Copy link
Member

@ngaut ngaut commented Dec 28, 2016

Close #1447

@siddontang
Copy link
Contributor

Should update the etc/config-template.toml too

@ngaut
Copy link
Member Author

ngaut commented Dec 28, 2016

PTAL

@siddontang
Copy link
Contributor

LGTM

# When region size changes exceeds region-split-check-diff, we should check
# whether the region should be split or not.
region-split-check-diff = "8MB"
region-split-check-diff = "24MB"
Copy link
Member

Choose a reason for hiding this comment

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

Why 24M

Copy link
Member Author

Choose a reason for hiding this comment

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

To reduce scanning times.

Copy link
Member

Choose a reason for hiding this comment

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

It will delay split scan.

const REGION_MAX_SIZE: u64 = 80 * 1024 * 1024;
const REGION_CHECK_DIFF: u64 = 8 * 1024 * 1024;
const REGION_SPLIT_SIZE: u64 = 96 * 1024 * 1024;
const REGION_MAX_SIZE: u64 = REGION_SPLIT_SIZE + REGION_SPLIT_SIZE / 3;
Copy link
Member

Choose a reason for hiding this comment

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

I think REGION_SPLIT_SIZE / 4 * 5 is more appropriate. REGION_SPLIT_SIZE can be divided by 4 or 8 normally.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't need to be accurate.

Copy link
Member

Choose a reason for hiding this comment

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

But it will look ugly when change the split size to something else, like 160m, the max size will become 213m in which case. But if using 5 / 4, default max size will become 200m.

const REGION_CHECK_DIFF: u64 = 8 * 1024 * 1024;
const REGION_SPLIT_SIZE: u64 = 96 * 1024 * 1024;
const REGION_MAX_SIZE: u64 = REGION_SPLIT_SIZE / 4 * 5;
const REGION_CHECK_DIFF: u64 = REGION_SPLIT_SIZE / 4;
Copy link
Member

Choose a reason for hiding this comment

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

Should be 8.


cfg.raft_store.region_check_size_diff =
get_toml_int(config,
"raftstore.region-split-check-diff",
Some(8 * 1024 * 1024)) as u64;
Some(24 * 1024 * 1024)) as u64;
Copy link
Member

Choose a reason for hiding this comment

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

Should be 16.

@siddontang
Copy link
Contributor

In our test, for 128MB region size, snapshot applying can cause IO higher and affect the raftstore thread.
I think we should support SST ASAP @hhkbp2.

@siddontang siddontang changed the title raftstore: increase region size from 64M to 96M [DNM] raftstore: increase region size from 64M to 96M Jan 3, 2017
@siddontang
Copy link
Contributor

@hhkbp2 seem we can use SST to test this now.

@hhkbp2
Copy link
Contributor

hhkbp2 commented Mar 27, 2017

LGTM
This PR is ready to go.

@ngaut ngaut changed the title [DNM] raftstore: increase region size from 64M to 96M raftstore: increase region size from 64M to 96M May 22, 2017
@ngaut
Copy link
Member Author

ngaut commented May 22, 2017

I prefer to merge the PR into 1.0.rc3, it's ready. PTAL

@siddontang
Copy link
Contributor

LGTM

@ngaut ngaut merged commit 4b999ea into master May 24, 2017
@ngaut ngaut deleted the ngaut/increase-region-size branch May 24, 2017 04:04
iosmanthus pushed a commit to iosmanthus/tikv that referenced this pull request May 31, 2024
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