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

Provide seeded KV generator #3853

Merged
merged 4 commits into from Nov 29, 2018

Conversation

Projects
None yet
4 participants
@breeswish
Member

breeswish commented Nov 28, 2018

Signed-off-by: Breezewish breezewish@pingcap.com

What have you changed? (mandatory)

This PR provides with_seed for KvGenerator so that it is possible to generate a reproduceable and stable KV pairs. It can eliminate performance jitters in benchmarks.

Extracted from #3477, also seems to be useful in #3787.

What are the type of the changes? (mandatory)

  • Improvement (non-breaking change which is an improvement to an existing feature)
Provide seeded KV generator
Signed-off-by: Breezewish <breezewish@pingcap.com>
@breeswish

This comment has been minimized.

Member

breeswish commented Nov 28, 2018

Fix
Signed-off-by: Breezewish <breezewish@pingcap.com>
@breeswish

This comment has been minimized.

Member

breeswish commented Nov 28, 2018

/run-integration-tests

/// Generate n pair of KVs.
///
/// This function consumes current generator.
pub fn generate(self, n: usize) -> Vec<(Vec<u8>, Vec<u8>)> {

This comment has been minimized.

@Hijiao

Hijiao Nov 28, 2018

Contributor

How about keep generate_random_kvs()? That is sample and useful, also I can add generate_deliberate_kvs() here.

This comment has been minimized.

@Hijiao

Hijiao Nov 28, 2018

Contributor
pub fn generate_deliberate_kvs(n: usize, key_len: usize, value_len: usize) -> Vec<(Vec<u8>, Vec<u8>)> {
    let kv_generator = KvGenerator::with_seed(key_len, value_len, 0);	
    kv_generator.take(n).collect()
}

This comment has been minimized.

@breeswish

breeswish Nov 28, 2018

Member

I think KvGenerator::new(..).generate(..) is more expressive and is not notably complex. Compared to the original generate_random_kvs, both of them only occupy 1 line with less than 50 characters. In addition, it is flexible enough: in this way, generate_deliberate_kvs is not needed because it can be just written as KvGenerator::from_seed(..).generate(..).

What do you think?

This comment has been minimized.

@Hijiao

Hijiao Nov 28, 2018

Contributor

Either is fine.

@Hijiao

Hijiao approved these changes Nov 28, 2018

LGTM

@rleungx rleungx added the S: LGT1 label Nov 28, 2018

@breeswish breeswish merged commit 047bf26 into tikv:master Nov 29, 2018

3 checks passed

DCO All commits are signed off!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
jenkins-ci-tikv/build Jenkins job succeeded.
Details

@breeswish breeswish deleted the breeswish:_kv_gen_with_seed branch Nov 29, 2018

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