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

added new cloud statistics facility. #25

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dlfurse
Copy link

@dlfurse dlfurse commented Jun 30, 2017

No description provided.

delete it;

// Flush all data from main db to sst files. Release db.
db->Flush(FlushOptions());
Copy link

@dhruba dhruba Jun 30, 2017

Choose a reason for hiding this comment

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

Can we print out the Statistics values into stdout? Then this example will be super clear that it is showing how Statistics work!

#include "rocksdb/options.h"

using namespace rocksdb;

Copy link

Choose a reason for hiding this comment

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

Don't you need to update examples/Makefile to compile and run this one?

*/
enum CloudTickers : uint32_t {
// # of times the manifest is written to the cloud
CLOUD_TICKER_ENUM_START = 1u << 31,
Copy link

Choose a reason for hiding this comment

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

Can we have some comments on why this needs to start at 1 << 31?

Copy link

@igorcanadi igorcanadi left a comment

Choose a reason for hiding this comment

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

Looks good overall, but couple of comments, thanks!

@@ -464,6 +465,13 @@ Status S3WritableFile::CopyManifestToS3(uint64_t size_hint, bool force) {
"[s3] S3WritableFile made manifest %s durable to "
"bucket %s bucketpath %s.",
fname_.c_str(), s3_bucket_.c_str(), s3_object_.c_str());

// If cloud stats are present, record the manifest write and its latency in millis.
auto stats = env_->cloud_env_options.cloud_statistics;

Choose a reason for hiding this comment

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

Pls use auto&. Otherwise, you'll copy shared_ptr here, which needs to write to a std::atomic.

Choose a reason for hiding this comment

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

On a related note, check out second paragraph here: http://smalldatum.blogspot.com/2016/10/make-myrocks-2x-less-slow.html

@@ -244,6 +251,9 @@ class AwsEnv : public CloudEnvImpl {
// The S3 client
std::shared_ptr<AwsS3ClientWrapper> s3client_;

// Results of last S3 client call; used in stats collection.
static thread_local AwsS3ClientResult s3client_result_;

Choose a reason for hiding this comment

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

Unfortunately we cannot use thread_local, since RocksDB can run on platforms that don't support it. We can use __thread, but only in some scenarios, check out how RocksDB does it: https://github.com/facebook/rocksdb/blob/master/monitoring/perf_context.cc#L20

auto stats = env_->cloud_env_options.cloud_statistics;
if (stats) {
stats->recordTick(NUMBER_MANIFEST_WRITES, 1);
stats->measureTime(MANIFEST_WRITES_TIME, env_->s3client_result_.micros / 1000);

Choose a reason for hiding this comment

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

Could you just measure time here instead of piggy-backing on the time measure in S3 callback? That would make the code much easier to reason about (no need for thread-local, wrapping the callback, etc). Also, you already have the start time of the operation recorded in line 455, so adding an extra time call shouldn't be a big concern.

@dhruba
Copy link

dhruba commented Jul 2, 2017

Hi @dlfurse, if you are ok with the review comments, please consider uploading a newer version of this patch. Then we can merge it into master.

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.

3 participants