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

[db_bench] Enable write rate limit for updaterandom benchmark #2578

Closed
wants to merge 1 commit into from

Conversation

yiwu-arbug
Copy link
Contributor

Summary:
We have FLAGS_benchmark_write_rate_limit to limit write rate in
db_bench, but it was not in use for updaterandom benchmark.

Test Plan:
Run the benchmark with FLAGS_benchmark_write_rate_limit on and check the
printed write rate.

Summary:
We have FLAGS_benchmark_write_rate_limit to limit write rate in
db_bench, but it was not in use for updaterandom benchmark.

Test Plan:
Run the benchmark with FLAGS_benchmark_write_rate_limit on and check the
printed write rate.
@@ -489,7 +489,8 @@ static bool ValidateCacheNumshardbits(const char* flagname, int32_t value) {
return true;
}

DEFINE_bool(verify_checksum, true, "Verify checksum for every block read"
DEFINE_bool(verify_checksum, true,
"Verify checksum for every block read"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

side effect of clang-format.

@facebook-github-bot
Copy link
Contributor

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

thread->shared->write_rate_limiter->Request(
key.size() + value_size_, Env::IO_HIGH, nullptr /*stats*/,
RateLimiter::OpType::kWrite);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at other uses of write_rate_limiter in this file, it seems like more needs to be done.
For example, if you look at TimeSeriesWrite, it seems to be doing:

    if (FLAGS_benchmark_write_rate_limit > 0) {
      write_rate_limiter.reset(
          NewGenericRateLimiter(FLAGS_benchmark_write_rate_limit));
    }

Don't you need to do something like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

May be I am wrong, but don't you need to check FLAGS_benchmark_write_rate_limit also and handle it, and not just if thread->shared->write_rate_limiter is not null?

Copy link
Contributor

Choose a reason for hiding this comment

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

nah, forget it. I'm wrong. Digging further I realized that all the things I mentioned above are already handled in RunBenchmark function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, maybe TimeSeriesWrite is using it in a wrong way..

@yiwu-arbug yiwu-arbug deleted the rate_limit branch July 14, 2017 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants