Skip to content

Commit

Permalink
Fix Statistics TickersNameMap miss match with Tickers enum
Browse files Browse the repository at this point in the history
Summary:
TickersNameMap is not consistent with Tickers enum.
this cause us to report wrong statistics and sometimes to access TickersNameMap outside it's boundary causing crashes (in Fb303 statistics)

Test Plan: added new unit test

Reviewers: sdong, kradhakrishnan

Reviewed By: kradhakrishnan

Subscribers: andrewkr, dhruba

Differential Revision: https://reviews.facebook.net/D61083
  • Loading branch information
IslamAbdelRahman committed Jul 25, 2016
1 parent 16e225f commit f8061a2
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 1 deletion.
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,7 @@ set(TESTS
util/options_test.cc
util/rate_limiter_test.cc
util/slice_transform_test.cc
util/statistics_test.cc
util/thread_list_test.cc
util/thread_local_test.cc
utilities/backupable/backupable_db_test.cc
Expand Down
4 changes: 4 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,7 @@ TESTS = \
ldb_cmd_test \
iostats_context_test \
persistent_cache_test \
statistics_test \

PARALLEL_TEST = \
backupable_db_test \
Expand Down Expand Up @@ -1220,6 +1221,9 @@ iostats_context_test: util/iostats_context_test.o $(LIBOBJECTS) $(TESTHARNESS)
persistent_cache_test: utilities/persistent_cache/persistent_cache_test.o db/db_test_util.o $(LIBOBJECTS) $(TESTHARNESS)
$(AM_LINK)

statistics_test: util/statistics_test.o $(LIBOBJECTS) $(TESTHARNESS)
$(AM_LINK)

#-------------------------------------------------
# make install related stuff
INSTALL_PATH ?= /usr/local
Expand Down
5 changes: 4 additions & 1 deletion include/rocksdb/statistics.h
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,8 @@ const std::vector<std::pair<Tickers, std::string>> TickersNameMap = {
{BLOCK_CACHE_BYTES_READ, "rocksdb.block.cache.bytes.read"},
{BLOCK_CACHE_BYTES_WRITE, "rocksdb.block.cache.bytes.write"},
{BLOOM_FILTER_USEFUL, "rocksdb.bloom.filter.useful"},
{PERSISTENT_CACHE_HIT, "rocksdb.persistent.cache.hit"},
{PERSISTENT_CACHE_MISS, "rocksdb.persistent.cache.miss"},
{MEMTABLE_HIT, "rocksdb.memtable.hit"},
{MEMTABLE_MISS, "rocksdb.memtable.miss"},
{GET_HIT_L0, "rocksdb.l0.hit"},
Expand Down Expand Up @@ -265,10 +267,11 @@ const std::vector<std::pair<Tickers, std::string>> TickersNameMap = {
{WAL_FILE_BYTES, "rocksdb.wal.bytes"},
{WRITE_DONE_BY_SELF, "rocksdb.write.self"},
{WRITE_DONE_BY_OTHER, "rocksdb.write.other"},
{WRITE_TIMEDOUT, "rocksdb.write.timeout"},
{WRITE_WITH_WAL, "rocksdb.write.wal"},
{FLUSH_WRITE_BYTES, "rocksdb.flush.write.bytes"},
{COMPACT_READ_BYTES, "rocksdb.compact.read.bytes"},
{COMPACT_WRITE_BYTES, "rocksdb.compact.write.bytes"},
{FLUSH_WRITE_BYTES, "rocksdb.flush.write.bytes"},
{NUMBER_DIRECT_LOAD_TABLE_PROPERTIES,
"rocksdb.number.direct.load.table.properties"},
{NUMBER_SUPERVERSION_ACQUIRES, "rocksdb.number.superversion_acquires"},
Expand Down
1 change: 1 addition & 0 deletions src.mk
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ MAIN_SOURCES = \
util/env_test.cc \
util/filelock_test.cc \
util/histogram_test.cc \
util/statistics_test.cc \
utilities/backupable/backupable_db_test.cc \
utilities/checkpoint/checkpoint_test.cc \
utilities/document/document_db_test.cc \
Expand Down
35 changes: 35 additions & 0 deletions util/statistics_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright (c) 2011-present, Facebook, Inc. All rights reserved.
// This source code is licensed under the BSD-style license found in the
// LICENSE file in the root directory of this source tree. An additional grant
// of patent rights can be found in the PATENTS file in the same directory.
//

#include "port/stack_trace.h"
#include "util/testharness.h"
#include "util/testutil.h"

#include "rocksdb/statistics.h"

namespace rocksdb {

class StatisticsTest : public testing::Test {};

// Sanity check to make sure that contents and order of TickersNameMap
// match Tickers enum
TEST_F(StatisticsTest, Sanity) {
EXPECT_EQ(static_cast<size_t>(Tickers::TICKER_ENUM_MAX),
TickersNameMap.size());

for (uint32_t t = 0; t < Tickers::TICKER_ENUM_MAX; t++) {
auto pair = TickersNameMap[static_cast<size_t>(t)];
ASSERT_EQ(pair.first, t) << "Miss match at " << pair.second;
}
}

} // namespace rocksdb

int main(int argc, char** argv) {
rocksdb::port::InstallStackTraceHandler();
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
}

0 comments on commit f8061a2

Please sign in to comment.