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

*: support split table #2378

Merged
merged 54 commits into from Nov 8, 2017

Conversation

Projects
None yet
4 participants
@overvenus
Member

overvenus commented Oct 12, 2017

Add TableCheckObserver for splitting regions by table.

@overvenus overvenus requested review from siddontang, BusyJay and disksing Oct 12, 2017

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Oct 13, 2017

Contributor

In TiKV design, the only coprocessor knows table, so it is bad to let raftstore knows it. I suggest:

  1. Move the split-table configuration to a coprocessor section.
  2. Create the CoprocessorHost outside and pass to raftstore
  3. Add a check_split_key like function in CoprocessorHost
  4. Register a real CrossTableChecker from the configuration to check the split key
Contributor

siddontang commented Oct 13, 2017

In TiKV design, the only coprocessor knows table, so it is bad to let raftstore knows it. I suggest:

  1. Move the split-table configuration to a coprocessor section.
  2. Create the CoprocessorHost outside and pass to raftstore
  3. Add a check_split_key like function in CoprocessorHost
  4. Register a real CrossTableChecker from the configuration to check the split key
@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Oct 13, 2017

Contributor

If a region crosses tables, we must split it even its size is less than the threshold.

Contributor

siddontang commented Oct 13, 2017

If a region crosses tables, we must split it even its size is less than the threshold.

@overvenus

This comment has been minimized.

Show comment
Hide comment
@overvenus

overvenus Oct 13, 2017

Member

@siddontang

If a region crosses tables, we must split it even its size is less than the threshold.

Yes, that is exactly how split worker behaves. PTAL at https://github.com/pingcap/tikv/pull/2378/files#diff-c14eb2753b62ec1bc97c7624b348e0f2R257

Member

overvenus commented Oct 13, 2017

@siddontang

If a region crosses tables, we must split it even its size is less than the threshold.

Yes, that is exactly how split worker behaves. PTAL at https://github.com/pingcap/tikv/pull/2378/files#diff-c14eb2753b62ec1bc97c7624b348e0f2R257

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Oct 13, 2017

Contributor

Here we will still check size at size at first.

Contributor

siddontang commented Oct 13, 2017

Here we will still check size at size at first.

@overvenus

This comment has been minimized.

Show comment
Hide comment
@overvenus

overvenus Oct 17, 2017

Member

PTAL

Member

overvenus commented Oct 17, 2017

PTAL

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Oct 17, 2017

Contributor

I prefer using checker chan here now, E.g., checkers [a, b]. So the logic is:

for c := in checkers {
   if !c.prev_check() {
         continue
   }
   c.split()
}

For our case, we can first use table checker, then size checker.

Contributor

siddontang commented Oct 17, 2017

I prefer using checker chan here now, E.g., checkers [a, b]. So the logic is:

for c := in checkers {
   if !c.prev_check() {
         continue
   }
   c.split()
}

For our case, we can first use table checker, then size checker.

@overvenus

This comment has been minimized.

Show comment
Hide comment
@overvenus

overvenus Oct 18, 2017

Member

PTAL

Member

overvenus commented Oct 18, 2017

PTAL

overvenus added a commit that referenced this pull request Nov 2, 2017

*: refactor split check (#2440)
Move size split check from SplitCheckRunner to CoprocessorHost.
It also move region-max-size and region-split-size to coprocessor
configuration.

Ref #2378

overvenus added some commits Nov 6, 2017

Show outdated Hide outdated src/raftstore/coprocessor/split_check/table.rs
}
#[test]
fn test_last_key_of_region() {

This comment has been minimized.

@BusyJay

BusyJay Nov 7, 2017

Contributor

This test is too complicated. Actually last_key_or_region has nothing to do with tables, so you can just test if it works when there is no keys, one key or multiple keys in the region.

@BusyJay

BusyJay Nov 7, 2017

Contributor

This test is too complicated. Actually last_key_or_region has nothing to do with tables, so you can just test if it works when there is no keys, one key or multiple keys in the region.

@disksing

This comment has been minimized.

Show comment
Hide comment
@disksing

disksing Nov 7, 2017

Contributor

LGTM.

Contributor

disksing commented Nov 7, 2017

LGTM.

@BusyJay

This comment has been minimized.

Show comment
Hide comment
@BusyJay

BusyJay Nov 7, 2017

Contributor

LGTM, @siddontang PTAL, we may need to refactor coprocessor later.

Contributor

BusyJay commented Nov 7, 2017

LGTM, @siddontang PTAL, we may need to refactor coprocessor later.

@siddontang

Rest LGTM

overvenus added some commits Nov 7, 2017

@overvenus

This comment has been minimized.

Show comment
Hide comment
@overvenus

overvenus Nov 7, 2017

Member

/run-all-tests

Member

overvenus commented Nov 7, 2017

/run-all-tests

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Nov 8, 2017

Contributor

CI failed @overvenus

Contributor

siddontang commented Nov 8, 2017

CI failed @overvenus

@overvenus

This comment has been minimized.

Show comment
Hide comment
@overvenus

overvenus Nov 8, 2017

Member

/run-all-tests

Member

overvenus commented Nov 8, 2017

/run-all-tests

@siddontang siddontang merged commit 64207ef into master Nov 8, 2017

5 of 7 checks passed

jenkins-ci-tikv/integration-common-test Jenkins job failed
Details
jenkins-ci-tikv/integration-ddl-test Jenkins job is running.
Details
ci/circleci Your tests passed on CircleCI!
Details
jenkins-ci-tikv/build Jenkins job succeeded.
Details
jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
jenkins-ci-tikv/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@siddontang siddontang deleted the ov/split-table branch Nov 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment