Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14920 )
Change subject: KUDU-2975: Spread WAL across multiple directories ...................................................................... Patch Set 6: (37 comments) http://gerrit.cloudera.org:8080/#/c/14920/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14920/5//COMMIT_MSG@19 PS5, Line 19: first we need use "kudu fs : update_dirs" tool to update the WAL dir. Is this actually the case though? Won't we just start up and use the new disks? http://gerrit.cloudera.org:8080/#/c/14920/5//COMMIT_MSG@30 PS5, Line 30: directorys's nit: directories' http://gerrit.cloudera.org:8080/#/c/14920/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14920/6//COMMIT_MSG@19 PS6, Line 19: first we need use "kudu fs : update_dirs" tool to update the WAL dir Is this actually the case? Won't Kudu just start up using the new directories even if didn't run the tool? http://gerrit.cloudera.org:8080/#/c/14920/6//COMMIT_MSG@26 PS6, Line 26: talbet's nit: tablet's http://gerrit.cloudera.org:8080/#/c/14920/6//COMMIT_MSG@26 PS6, Line 26: But flushing : metadata may causes a deadlock in the tool code, so we don't persist : the metadata immediately. nit: you can probably omit this and instead describe what we do: "We'll first register the WAL directory in memory, and the next time the metadata is flushed, the directory's UUID is persisted." http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/consensus/log.h File src/kudu/consensus/log.h: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/consensus/log.h@322 PS6, Line 322: wal_path nit: could you also describe 'wal_path' and how it can be empty? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/consensus/log.cc File src/kudu/consensus/log.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/consensus/log.cc@1208 PS6, Line 1208: //Status s = fs_manager->GetTabletWalRecoveryDir(dir_uuid, tablet_id, &recovery_path); Remove? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/fs_manager-test.cc@166 PS6, Line 166: : TEST_F(FsManagerTestBase, TestMultipleWalPaths) { : vector<string> wal_paths = { GetTestPath("a"), GetTestPath("b"), GetTestPath("c") }; : vector<string> data_paths = { GetTestPath("a"), GetTestPath("b"), GetTestPath("c") }; : ReinitFsManagerWithPaths(wal_paths, data_paths); : ASSERT_OK(fs_manager()->CreateInitialFileSystemLayout()); : ASSERT_OK(fs_manager()->Open()); : } nit: maybe just lump this in with TestMultiplePaths? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/fs_manager-test.cc@181 PS6, Line 181: ) Would it make sense to test the extra slashes here too? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/fs_manager-test.cc@296 PS6, Line 296: TEST_F(FsManagerTestBase, TestFormatWithSpecificUUID) { : string path = GetTestPath("new_fs_root"); : string path2 = GetTestPath("new_fs_root2"); : ReinitFsManagerWithPaths({ path, path2 }, {}); nit: probably don't need to update this test http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/fs_manager-test.cc@872 PS6, Line 872: // There is a file "wal_manager_instance" in the wals directory, cannot : // use DeleteDir any more. nit: maybe write this to be more stateless, e.g. "There should be a 'wal_manager_instance' so we need to delete directory recursively." http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/fs_manager-test.cc@1384 PS6, Line 1384: TEST_F(FsManagerTestBase, TestAddRemoveWalDirsFuzz) { I wonder if, instead of copying TestAddRemoveDataDirsFuzz completely, whether we can merge the tests by doing Uniform(4) and having fource cases: {add data dir, remove data dir, add wal dir, remove wal dir}. That way we wouldn't duplicate such a complex test, and it would add some test coverage for cases for mixing adding/removing data directories and adding/removing WAL directories. http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/fs_manager.cc@682 PS6, Line 682: data nit: wal http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/fs_manager.cc@795 PS6, Line 795: // Temporary files in the Block Manager directories are cleaned during : // Block Manager startup. This should be true for the WAL manager too, right? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs-test.cc File src/kudu/fs/wal_dirs-test.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs-test.cc@158 PS6, Line 158: /* : TEST_F(WalDirManagerTest, TestUpdateDirsFromOldVersion) { : // Remove the "wal_manager_instance" file under "wals" directory. : string manger_instance_file = JoinPathSegments( : JoinPathSegments(test_roots_[0], kWalDirName), kWalInstanceMetadataFileName); : ASSERT_OK(env_->DeleteFile(manger_instance_file)); : for (int i = 1; i < kNumDirs; ++i) { : ASSERT_OK(env_->DeleteRecursively(JoinPathSegments(test_roots_[i], kWalDirName))); : } : WalDirManagerOptions opts; : opts.update_instances = UpdateInstanceBehavior::UPDATE_AND_ERROR_ON_FAILURE; : // The directory manager will successfully open with the single failed directory. : ASSERT_OK(WalDirManager::OpenExistingForTests(env_, test_roots_, : opts, &wd_manager_)); : ASSERT_EQ(wd_manager_->GetRoots().size(), kNumDirs); : ASSERT_EQ(wd_manager_->dirs().size(), kNumDirs); : ASSERT_EQ(wd_manager_->GetFailedDirs().size(), 0); : } : */ Should this be removed? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs-test.cc@226 PS6, Line 226: $0-$1" nit: maybe create a separate container for the tablets? E.g. vector<string> tablets(kNumDirs); for (int i = 0; i < kNumDirs; i++) { tablets[i] = Substitute("$0-$1", test_tablet_name_, i); } Then we can reuse those values elsewhere in this test. http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs-test.cc@236 PS6, Line 236: for (int i = 0; i < kNumDirs; ++i) { nit: So it's easier to understand the rationale of this test, could you add a comment explaining, something along the lines of: Create an extra WAL directory on disk for the tablets, and unregister each tablet's WAL directory in memory. If we try searching for each tablet's WAL directory on disk, we should fail because there are multiple WAL directories per tablet. http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs-test.cc@236 PS6, Line 236: for (int i = 0; i < kNumDirs; ++i) { : ASSERT_OK(env_->CreateDir(JoinPathSegments(test_roots_[kNumDirs - i - 1], : Substitute("wals/$0-$1", test_tablet_name_, i)))); : wd_manager_->DeleteWalDir(Substitute("$0-$1", test_tablet_name_, i)); : } : for (int i = 0; i < kNumDirs; ++i) { : Status s = wd_manager_->FindAndRegisterWalDirOnDisk(Substitute( : "$0-$1", test_tablet_name_, i)); : ASSERT_STR_CONTAINS(s.ToString(), "has at least two WAL directories"); : ASSERT_TRUE(s.IsCorruption()); : } Could these be in the same for loop? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs-test.cc@248 PS6, Line 248: InsertOrDie(&wd_manager_->uuid_idx_by_tablet_, Substitute( : "$0-$1", test_tablet_name_, i), i); nit: how about calling CreateWalDir() instead so we exercise the public API instead? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs-test.cc@247 PS6, Line 247: for (int i = 0; i < kNumDirs; ++i) { : InsertOrDie(&wd_manager_->uuid_idx_by_tablet_, Substitute( : "$0-$1", test_tablet_name_, i), i); : } : for (int i = 0; i < kNumDirs; ++i) { : Status s = wd_manager_->FindAndRegisterWalDirOnDisk(Substitute( : "$0-$1", test_tablet_name_, i)); : ASSERT_TRUE(s.IsAlreadyPresent()); : } Could these be in the same for loop? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs-test.cc@328 PS6, Line 328: TEST_F(WalDirManagerTest, TestFullDisk) { Should we also check the the wal_dirs_full metric gets updated? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs-test.cc@346 PS6, Line 346: // Test that concurrently create wal dirs for tablets yields the expected : // number of dirs added. Should we verify the directories? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs-test.cc@407 PS6, Line 407: // Calculate the standard deviation of the number of tablets per disk. : // If tablets are evenly spread across directories, this should be small. This isn't true anymore. Mind updating it? W're not looking at standard deviation here. Same with the below test. http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs.h File src/kudu/fs/wal_dirs.h: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs.h@131 PS6, Line 131: // Deletes the WAL directory for the specified tablet. Maps from tablet_id to nit: can you specify that it only unregisters the directory in memory? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs.cc File src/kudu/fs/wal_dirs.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs.cc@359 PS6, Line 359: string uuid; : if (!FindCopy(uuid_by_idx_, *uuid_idx, &uuid)) { : return Status::NotFound(Substitute( : "could not find data dir with uuid index $0", *uuid_idx)); : } I thought a UUID index must always correspond to a UUID, so this case should be impossible, right? Could we use FindOrDie(uuid_by_idx_, *uuid_idx) instead? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs.cc@378 PS6, Line 378: if (!wd) { : return Status::NotFound(Substitute( : "could not find wal dir for tablet $0", tablet_id)); : } I thought a UUID index must always correspond to a directory, so this case should be impossible, right? Could we use FindOrDie(dir_by_uuid_idx_, *uuid_idx) instead? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tablet/tablet_bootstrap-test.cc File src/kudu/tablet/tablet_bootstrap-test.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tablet/tablet_bootstrap-test.cc@246 PS6, Line 246: meatadta nit: metadata http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tablet/tablet_bootstrap-test.cc@284 PS6, Line 284: } : : Maybe also check that the WAL directory UUID is the same after reloading? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tablet/tablet_metadata-test.cc File src/kudu/tablet/tablet_metadata-test.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tablet/tablet_metadata-test.cc@182 PS6, Line 182: superblock_pb_0 nit: you could just reuse the same TabletSuperBlockPB member everywhere, right? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tablet/tablet_metadata-test.cc@193 PS6, Line 193: harness_->tablet() nit: maybe capture this in a local Tablet* variable http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tablet/tablet_metadata-test.cc@203 PS6, Line 203: finded nit: found http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tablet/tablet_metadata.cc@467 PS6, Line 467: WalDir nit: "WAL directory" so it's consistent with other messages http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tools/tool_action_fs.cc@349 PS6, Line 349: s1.ToString(); Maybe print both? http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tools/tool_action_local_replica.cc@523 PS6, Line 523: // For new version, need load metadata, find directory from metadata. : // But we just find the wal dir for tablet from disk first. nit: maybe reword a bit: "Look for a WAL directory, consulting disk if necessary." http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tserver/tablet_server-test-base.h File src/kudu/tserver/tablet_server-test-base.h: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tserver/tablet_server-test-base.h@64 PS6, Line 64: nit: fix spacing http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tserver/ts_tablet_manager.cc@1465 PS6, Line 1465: string wal_dir; nit: could you add a comment why we're doing this before deleting the metadata? e.g. "We must delete the WAL after deleting the metadata. But the process of deleting metadata will unregister this tablet's WAL directory. So make copies now so we can delete them later." http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tserver/ts_tablet_manager.cc@1484 PS6, Line 1484: CHECK_OK(Log::DeleteOnDiskData(meta->fs_manager(), wal_dir, tablet_id)); : CHECK_OK(Log::RemoveRecoveryDirIfExists(meta->fs_manager(), wal_recovery_dir, : tablet_id)); nit: maybe condition on !wal_dir.empty() and !wal_recovery_dir.empty() respectively? -- To view, visit http://gerrit.cloudera.org:8080/14920 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic3490b2bbc0544e7c8a5f987fb83855ff155ac2c Gerrit-Change-Number: 14920 Gerrit-PatchSet: 6 Gerrit-Owner: YangSong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong <[email protected]> Gerrit-Comment-Date: Fri, 10 Jan 2020 03:35:36 +0000 Gerrit-HasComments: Yes
