From e1b3ee8a799f155d7c0a99f2e25bc72c54d4c875 Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Thu, 7 Jul 2016 11:35:40 -0700 Subject: [PATCH] Cleanup auto-roll logger flush-while-rolling test Summary: Use @omegaga's awesome feature to avoid use of callbacks for ensuring SyncPoints happen in a particular thread. Depends on D60375. Test Plan: $ ./auto_roll_logger_test Reviewers: omegaga, sdong Reviewed By: sdong Subscribers: andrewkr, dhruba, omegaga, leveldb Differential Revision: https://reviews.facebook.net/D60471 --- db/auto_roll_logger_test.cc | 47 ++++++++++++++----------------------- util/posix_logger.h | 3 ++- 2 files changed, 20 insertions(+), 30 deletions(-) diff --git a/db/auto_roll_logger_test.cc b/db/auto_roll_logger_test.cc index 6c7293029..6a0c95461 100644 --- a/db/auto_roll_logger_test.cc +++ b/db/auto_roll_logger_test.cc @@ -273,35 +273,24 @@ TEST_F(AutoRollLoggerTest, LogFlushWhileRolling) { ASSERT_TRUE(auto_roll_logger); std::thread flush_thread; - rocksdb::SyncPoint::GetInstance()->LoadDependency({ - // Need to pin the old logger before beginning the roll, as rolling grabs - // the mutex, which would prevent us from accessing the old logger. - {"AutoRollLogger::Flush:PinnedLogger", - "AutoRollLoggerTest::LogFlushWhileRolling:PreRollAndPostThreadInit"}, - // Need to finish the flush thread init before this callback because the - // callback accesses flush_thread.get_id() in order to apply certain sync - // points only to the flush thread. - {"AutoRollLoggerTest::LogFlushWhileRolling:PreRollAndPostThreadInit", - "AutoRollLoggerTest::LogFlushWhileRolling:FlushCallbackBegin"}, - // Need to reset logger at this point in Flush() to exercise a race - // condition case, which is executing the flush with the pinned (old) - // logger after the roll has cut over to a new logger. - {"AutoRollLoggerTest::LogFlushWhileRolling:FlushCallback1", - "AutoRollLogger::ResetLogger:BeforeNewLogger"}, - {"AutoRollLogger::ResetLogger:AfterNewLogger", - "AutoRollLoggerTest::LogFlushWhileRolling:FlushCallback2"}, - }); - rocksdb::SyncPoint::GetInstance()->SetCallBack( - "PosixLogger::Flush:BeginCallback", [&](void* arg) { - TEST_SYNC_POINT( - "AutoRollLoggerTest::LogFlushWhileRolling:FlushCallbackBegin"); - if (std::this_thread::get_id() == flush_thread.get_id()) { - TEST_SYNC_POINT( - "AutoRollLoggerTest::LogFlushWhileRolling:FlushCallback1"); - TEST_SYNC_POINT( - "AutoRollLoggerTest::LogFlushWhileRolling:FlushCallback2"); - } - }); + // Notes: + // (1) Need to pin the old logger before beginning the roll, as rolling grabs + // the mutex, which would prevent us from accessing the old logger. This + // also marks flush_thread with AutoRollLogger::Flush:PinnedLogger. + // (2) Need to reset logger during PosixLogger::Flush() to exercise a race + // condition case, which is executing the flush with the pinned (old) + // logger after auto-roll logger has cut over to a new logger. + // (3) PosixLogger::Flush() happens in both threads but its SyncPoints only + // are enabled in flush_thread (the one pinning the old logger). + rocksdb::SyncPoint::GetInstance()->LoadDependencyAndMarkers( + {{"AutoRollLogger::Flush:PinnedLogger", + "AutoRollLoggerTest::LogFlushWhileRolling:PreRollAndPostThreadInit"}, + {"PosixLogger::Flush:Begin1", + "AutoRollLogger::ResetLogger:BeforeNewLogger"}, + {"AutoRollLogger::ResetLogger:AfterNewLogger", + "PosixLogger::Flush:Begin2"}}, + {{"AutoRollLogger::Flush:PinnedLogger", "PosixLogger::Flush:Begin1"}, + {"AutoRollLogger::Flush:PinnedLogger", "PosixLogger::Flush:Begin2"}}); rocksdb::SyncPoint::GetInstance()->EnableProcessing(); flush_thread = std::thread([&]() { auto_roll_logger->Flush(); }); diff --git a/util/posix_logger.h b/util/posix_logger.h index f9b93afdb..ddad037e0 100644 --- a/util/posix_logger.h +++ b/util/posix_logger.h @@ -55,7 +55,8 @@ class PosixLogger : public Logger { fclose(file_); } virtual void Flush() override { - TEST_SYNC_POINT_CALLBACK("PosixLogger::Flush:BeginCallback", nullptr); + TEST_SYNC_POINT("PosixLogger::Flush:Begin1"); + TEST_SYNC_POINT("PosixLogger::Flush:Begin2"); if (flush_pending_) { flush_pending_ = false; fflush(file_);