Repository: kudu Updated Branches: refs/heads/master 15afb6495 -> 3f194c3a1
test_util: reset flags before cleaning test directory TabletServerDiskFailureTest.TestRandomOpSequence didn't reset env_inject_eio to 0 before finishing. Rather than fixing that, let's change KuduTest to reset its flags _before_ cleaning out the test directory. This obviates the need for all such workarounds. Change-Id: I4b4843cbc2c3bc2cbeb3a4d5959f80a35da7714a Reviewed-on: http://gerrit.cloudera.org:8080/9944 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong <aw...@cloudera.com> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/3f194c3a Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/3f194c3a Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/3f194c3a Branch: refs/heads/master Commit: 3f194c3a17f11a70e64bf2668a96e1e93fed00df Parents: 15afb64 Author: Adar Dembo <a...@cloudera.com> Authored: Fri Apr 6 10:41:26 2018 -0700 Committer: Adar Dembo <a...@cloudera.com> Committed: Fri Apr 6 21:03:38 2018 +0000 ---------------------------------------------------------------------- src/kudu/fs/block_manager-test.cc | 2 -- src/kudu/fs/data_dirs-test.cc | 1 - src/kudu/fs/fs_manager-test.cc | 24 +++++++++--------------- src/kudu/fs/log_block_manager-test.cc | 14 ++++++++------ src/kudu/tablet/diskrowset-test.cc | 2 -- src/kudu/util/test_util.cc | 4 ++++ src/kudu/util/test_util.h | 12 ++++++++++-- 7 files changed, 31 insertions(+), 28 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/3f194c3a/src/kudu/fs/block_manager-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/fs/block_manager-test.cc b/src/kudu/fs/block_manager-test.cc index e17a61e..9d5a590 100644 --- a/src/kudu/fs/block_manager-test.cc +++ b/src/kudu/fs/block_manager-test.cc @@ -998,7 +998,6 @@ TYPED_TEST(BlockManagerTest, TestMetadataOkayDespiteFailure) { false /* create */)); } } - FLAGS_env_inject_eio = 0; } TYPED_TEST(BlockManagerTest, TestGetAllBlockIds) { @@ -1147,7 +1146,6 @@ TYPED_TEST(BlockManagerTest, TestBlockTransaction) { ASSERT_STR_CONTAINS(s.ToString(), Substitute("only deleted $0 blocks, " "first failure", deleted_blocks.size())); - FLAGS_env_inject_eio = 0; } } // namespace fs http://git-wip-us.apache.org/repos/asf/kudu/blob/3f194c3a/src/kudu/fs/data_dirs-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/fs/data_dirs-test.cc b/src/kudu/fs/data_dirs-test.cc index 449780a..7d35f09 100644 --- a/src/kudu/fs/data_dirs-test.cc +++ b/src/kudu/fs/data_dirs-test.cc @@ -421,7 +421,6 @@ TEST_F(DataDirManagerTest, TestOpenWithFailedDirs) { DataDirManagerOptions(), &dd_manager_); ASSERT_STR_CONTAINS(s.ToString(), "could not open directory manager"); ASSERT_TRUE(s.IsIOError()); - FLAGS_env_inject_eio = 0; } class TooManyDataDirManagerTest : public DataDirManagerTest { http://git-wip-us.apache.org/repos/asf/kudu/blob/3f194c3a/src/kudu/fs/fs_manager-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/fs/fs_manager-test.cc b/src/kudu/fs/fs_manager-test.cc index 4952a6b..5a76883 100644 --- a/src/kudu/fs/fs_manager-test.cc +++ b/src/kudu/fs/fs_manager-test.cc @@ -406,7 +406,6 @@ TEST_F(FsManagerTestBase, TestCreateWithFailedDirs) { Status s = fs_manager()->CreateInitialFileSystemLayout(); ASSERT_STR_MATCHES(s.ToString(), "cannot create FS layout; at least one directory " "failed to canonicalize"); - FLAGS_env_inject_eio = 0; } TEST_F(FsManagerTestBase, TestOpenWithNoBlockManagerInstances) { @@ -458,8 +457,6 @@ TEST_F(FsManagerTestBase, TestOpenWithFailedDirs) { ReinitFsManagerWithPaths(wal_path, data_roots); ASSERT_OK(fs_manager()->Open(nullptr)); ASSERT_EQ(1, fs_manager()->dd_manager()->GetFailedDataDirs().size()); - - FLAGS_env_inject_eio = 0; } TEST_F(FsManagerTestBase, TestTmpFilesCleanup) { @@ -633,19 +630,16 @@ TEST_F(FsManagerTestBase, TestAddRemoveDataDirs) { // Try to open with a new data dir although an existing data dir has failed; // this should fail. - { - google::FlagSaver saver; - FLAGS_crash_on_eio = false; - FLAGS_env_inject_eio = 1.0; - FLAGS_env_inject_eio_globs = JoinPathSegments(new_path2, "**"); + FLAGS_crash_on_eio = false; + FLAGS_env_inject_eio = 1.0; + FLAGS_env_inject_eio_globs = JoinPathSegments(new_path2, "**"); - const string new_path3 = GetTestPath("new_path3"); - opts.data_roots = { fs_root_, new_path2, new_path3 }; - ReinitFsManagerWithOpts(opts); - Status s = fs_manager()->Open(); - ASSERT_TRUE(s.IsIOError()); - ASSERT_STR_CONTAINS(s.ToString(), "found failed data directory"); - } + const string new_path3 = GetTestPath("new_path3"); + opts.data_roots = { fs_root_, new_path2, new_path3 }; + ReinitFsManagerWithOpts(opts); + s = fs_manager()->Open(); + ASSERT_TRUE(s.IsIOError()); + ASSERT_STR_CONTAINS(s.ToString(), "found failed data directory"); } TEST_F(FsManagerTestBase, TestReAddRemovedDataDir) { http://git-wip-us.apache.org/repos/asf/kudu/blob/3f194c3a/src/kudu/fs/log_block_manager-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/fs/log_block_manager-test.cc b/src/kudu/fs/log_block_manager-test.cc index 460d9d4..d93d8aa 100644 --- a/src/kudu/fs/log_block_manager-test.cc +++ b/src/kudu/fs/log_block_manager-test.cc @@ -30,6 +30,7 @@ #include <vector> #include <boost/optional/optional.hpp> +#include <gflags/gflags.h> #include <gflags/gflags_declare.h> #include <glog/logging.h> #include <gtest/gtest.h> @@ -1016,11 +1017,13 @@ TEST_F(LogBlockManagerTest, TestFailMultipleTransactionsPerContainer) { // Briefly inject an error while committing one of the transactions. This // should make the container read-only, preventing the remaining transactions // from proceeding. - FLAGS_crash_on_eio = false; - FLAGS_env_inject_eio = 1.0; - Status s = block_transactions[0]->CommitCreatedBlocks(); - ASSERT_TRUE(s.IsIOError()); - FLAGS_env_inject_eio = 0; + { + google::FlagSaver saver; + FLAGS_crash_on_eio = false; + FLAGS_env_inject_eio = 1.0; + Status s = block_transactions[0]->CommitCreatedBlocks(); + ASSERT_TRUE(s.IsIOError()); + } // Now try to add some more blocks. for (int i = 0; i < kNumTransactions; i++) { @@ -1647,7 +1650,6 @@ TEST_F(LogBlockManagerTest, TestOpenWithFailedDirectories) { int uuid_idx; dd_manager_->FindUuidIndexByRoot(test_dirs[failed_idx], &uuid_idx); ASSERT_TRUE(ContainsKey(failed_dirs, uuid_idx)); - FLAGS_env_inject_eio = 0; } // Test Close() a FINALIZED block. Including, http://git-wip-us.apache.org/repos/asf/kudu/blob/3f194c3a/src/kudu/tablet/diskrowset-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tablet/diskrowset-test.cc b/src/kudu/tablet/diskrowset-test.cc index 3005512..d6e9363 100644 --- a/src/kudu/tablet/diskrowset-test.cc +++ b/src/kudu/tablet/diskrowset-test.cc @@ -223,8 +223,6 @@ TEST_F(TestRowSet, TestErrorDuringUpdate) { Status s = rs->MutateRow(timestamp, probe, enc.as_changelist(), op_id_, &stats, &result); LOG(INFO) << s.ToString(); ASSERT_TRUE(s.IsIOError()); - - FLAGS_env_inject_eio = 0; } TEST_F(TestRowSet, TestRandomRead) { http://git-wip-us.apache.org/repos/asf/kudu/blob/3f194c3a/src/kudu/util/test_util.cc ---------------------------------------------------------------------- diff --git a/src/kudu/util/test_util.cc b/src/kudu/util/test_util.cc index b9f919a..30726a5 100644 --- a/src/kudu/util/test_util.cc +++ b/src/kudu/util/test_util.cc @@ -78,6 +78,7 @@ bool g_is_gtest = true; KuduTest::KuduTest() : env_(Env::Default()), + flag_saver_(new google::FlagSaver()), test_dir_(GetTestDataDirectory()) { std::map<const char*, const char*> flags_for_tests = { // Disabling fsync() speeds up tests dramatically, and it's safe to do as no @@ -108,6 +109,9 @@ KuduTest::KuduTest() } KuduTest::~KuduTest() { + // Reset the flags first to prevent them from affecting test directory cleanup. + flag_saver_.reset(); + // Clean up the test directory in the destructor instead of a TearDown // method. This is better because it ensures that the child-class // dtor runs first -- so, if the child class is using a minicluster, etc, http://git-wip-us.apache.org/repos/asf/kudu/blob/3f194c3a/src/kudu/util/test_util.h ---------------------------------------------------------------------- diff --git a/src/kudu/util/test_util.h b/src/kudu/util/test_util.h index f79daa6..67f0034 100644 --- a/src/kudu/util/test_util.h +++ b/src/kudu/util/test_util.h @@ -23,9 +23,9 @@ #include <cstdint> #include <functional> +#include <memory> #include <string> -#include <gflags/gflags.h> #include <gtest/gtest.h> #include "kudu/gutil/port.h" @@ -36,6 +36,10 @@ NO_PENDING_FATALS(); \ } while (0) +namespace google { +class FlagSaver; +} // namespace google + namespace kudu { class Env; @@ -64,7 +68,11 @@ class KuduTest : public ::testing::Test { std::string GetTestPath(const std::string& relative_path); Env* env_; - google::FlagSaver flag_saver_; // Reset flags on every test. + + // Reset flags on every test. Allocated on the heap so it can be destroyed + // (and the flags reset) before test_dir_ is deleted. + std::unique_ptr<google::FlagSaver> flag_saver_; + std::string test_dir_; };
