Fix valgrind errors in rocksdb tests: auto_roll_logger_test, reduce_levels_test

Summary: Fix for memory leaks in rocksdb tests. Also modified the variable NUM_FAILED_TESTS to print the actual number of failed tests.

Test Plan: make <test>; valgrind --leak-check=full ./<test>

Reviewers: sheki, dhruba

Reviewed By: sheki

CC: leveldb

Differential Revision: https://reviews.facebook.net/D9333

db/version_set_reduce_num_levels.cc

@@ -67,7 +67,8 @@ Status VersionSet::ReduceNumberOfLevels(int new_levels, port::Mutex* mu) {
   num_levels_ = new_levels;
   compact_pointer_ = new std::string[new_levels];
   Init(new_levels);
-  st = LogAndApply(new VersionEdit(new_levels), mu, true);
+  VersionEdit ve(new_levels);
+  st = LogAndApply(&ve , mu, true);
   return st;
 }
 

util/auto_roll_logger_test.cc

@@ -122,13 +122,11 @@ TEST(AutoRollLoggerTest, RollLogFileBySize) {
     InitTestDb();
     size_t log_max_size = 1024 * 5;
 
-    AutoRollLogger* logger = new AutoRollLogger(
-        Env::Default(), kTestDir, "", log_max_size, 0);
+    AutoRollLogger logger(Env::Default(), kTestDir, "", log_max_size, 0);
 
-    RollLogFileBySizeTest(logger, log_max_size,
+    RollLogFileBySizeTest(&logger, log_max_size,
                           kSampleMessage + ":RollLogFileBySize");
 
-    delete logger;
 }
 
 TEST(AutoRollLoggerTest, RollLogFileByTime) {
@@ -138,13 +136,10 @@ TEST(AutoRollLoggerTest, RollLogFileByTime) {
     InitTestDb();
     // -- Test the existence of file during the server restart.
     ASSERT_TRUE(!env->FileExists(kLogFile));
-    AutoRollLogger* logger = new AutoRollLogger(
-        Env::Default(), kTestDir, "", log_size, 1);
+    AutoRollLogger logger(Env::Default(), kTestDir, "", log_size, 1);
     ASSERT_TRUE(env->FileExists(kLogFile));
 
-    RollLogFileByTimeTest(logger, time, kSampleMessage + ":RollLogFileByTime");
-
-    delete logger;
+    RollLogFileByTimeTest(&logger, time, kSampleMessage + ":RollLogFileByTime");
 }
 
 TEST(AutoRollLoggerTest,
@@ -178,16 +173,15 @@ TEST(AutoRollLoggerTest, CompositeRollByTimeAndSizeLogger) {
 
   InitTestDb();
 
-  AutoRollLogger* logger = new AutoRollLogger(
-        Env::Default(), kTestDir, "", log_max_size, time);
+  AutoRollLogger logger(Env::Default(), kTestDir, "", log_max_size, time);
 
   // Test the ability to roll by size
   RollLogFileBySizeTest(
-      logger, log_max_size,
+      &logger, log_max_size,
       kSampleMessage + ":CompositeRollByTimeAndSizeLogger");
 
   // Test the ability to roll by Time
-  RollLogFileByTimeTest( logger, time,
+  RollLogFileByTimeTest( &logger, time,
       kSampleMessage + ":CompositeRollByTimeAndSizeLogger");
 }
 

util/ldb_cmd.cc

@@ -392,7 +392,7 @@ void DBLoaderCommand::DoCommand() {
     std::cout << "Warning: " << bad_lines << " bad lines ignored." << std::endl;
   }
   if (compact_) {
-    db_->CompactRange(NULL, NULL);
+    db_->CompactRange(nullptr, nullptr);
   }
 }
 
@@ -560,27 +560,24 @@ leveldb::Options ReduceDBLevelsCommand::PrepareOptionsForOpenDB() {
 
 Status ReduceDBLevelsCommand::GetOldNumOfLevels(leveldb::Options& opt,
     int* levels) {
-  TableCache* tc = new TableCache(db_path_, &opt, 10);
-  const InternalKeyComparator* cmp = new InternalKeyComparator(
-      opt.comparator);
-  VersionSet* versions = new VersionSet(db_path_, &opt,
-                                   tc, cmp);
+  TableCache tc(db_path_, &opt, 10);
+  const InternalKeyComparator cmp(opt.comparator);
+  VersionSet versions(db_path_, &opt, &tc, &cmp);
   // We rely the VersionSet::Recover to tell us the internal data structures
   // in the db. And the Recover() should never do any change
   // (like LogAndApply) to the manifest file.
-  Status st = versions->Recover();
+  Status st = versions.Recover();
   if (!st.ok()) {
     return st;
   }
   int max = -1;
-  for (int i = 0; i < versions->NumberLevels(); i++) {
-    if (versions->NumLevelFiles(i)) {
+  for (int i = 0; i < versions.NumberLevels(); i++) {
+    if (versions.NumLevelFiles(i)) {
       max = i;
     }
   }
 
   *levels = max + 1;
-  delete versions;
   return st;
 }
 
@@ -619,15 +616,13 @@ void ReduceDBLevelsCommand::DoCommand() {
   db_->CompactRange(nullptr, nullptr);
   CloseDB();
 
-  TableCache* tc = new TableCache(db_path_, &opt, 10);
-  const InternalKeyComparator* cmp = new InternalKeyComparator(
-      opt.comparator);
-  VersionSet* versions = new VersionSet(db_path_, &opt,
-                                   tc, cmp);
+  TableCache tc(db_path_, &opt, 10);
+  const InternalKeyComparator cmp(opt.comparator);
+  VersionSet versions(db_path_, &opt, &tc, &cmp);
   // We rely the VersionSet::Recover to tell us the internal data structures
   // in the db. And the Recover() should never do any change (like LogAndApply)
   // to the manifest file.
-  st = versions->Recover();
+  st = versions.Recover();
   if (!st.ok()) {
     exec_state_ = LDBCommandExecuteResult::FAILED(st.ToString());
     return;
@@ -635,7 +630,7 @@ void ReduceDBLevelsCommand::DoCommand() {
 
   port::Mutex mu;
   mu.Lock();
-  st = versions->ReduceNumberOfLevels(new_levels_, &mu);
+  st = versions.ReduceNumberOfLevels(new_levels_, &mu);
   mu.Unlock();
 
   if (!st.ok()) {

valgrind_test.sh

@@ -4,7 +4,7 @@
 
 VALGRIND_DIR=VALGRIND_LOGS
 make -j$(nproc) valgrind_check
-NUM_FAILED_TESTS=`wc -l $VALGRIND_DIR/valgrind_failed_tests | awk '{print $1}'`
+NUM_FAILED_TESTS=$((`wc -l $VALGRIND_DIR/valgrind_failed_tests | awk '{print $1}'` - 1))
 if [ $NUM_FAILED_TESTS -le 1 ]; then
   echo No tests have valgrind errors
   exit 0