Adar Dembo has posted comments on this change. Change subject: disk failure: add persistent disk states ......................................................................
Patch Set 15: (30 comments) Made it further in. Todd should review this too, since he was in that meeting we had to discuss this approach (a month or so ago). http://gerrit.cloudera.org:8080/#/c/7270/15/src/kudu/fs/block_manager_util-test.cc File src/kudu/fs/block_manager_util-test.cc: Line 151: TEST_F(KuduTest, CheckIntegrity) { There's a lot to process here. In total, do you get full coverage of every single path out of PathInstanceMetadataFile::CheckIntegrity? And full coverage of the various "update these instance files" branches too? http://gerrit.cloudera.org:8080/#/c/7270/6/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: Line 88: > Good point, that should work. I don't see this change in PS15. http://gerrit.cloudera.org:8080/#/c/7270/15/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: PS15, Line 54: CloneAndPrepend(msg) CloneAndPrepend() is work; can we defer it to if !s.ok() and we want to return the error? PS15, Line 69: Status::OK() This is the default value anyway; you can omit health_status_ from the list. PS15, Line 121: // If the write fails, don't update the timestamp. : uint64_t original_timestamp = pb->path_set().timestamp_us(); : auto reset_timestamp = MakeScopedCleanup([&] { : pb->mutable_path_set()->set_timestamp_us(original_timestamp); : }); This doesn't seem that important, but OK. PS15, Line 137: // Some instances on failed disks (e.g. those that failed to canonicalize : // their path) are prefixed with illegal whitespaces to signify failure. As we discussed offline, rather than modifying the path to indicate a failure in canonicalization, let's pass that information along separately (perhaps by passing the raw canonicalization status of each path). Then we won't need this hack, and we probably won't need to sanitize here at all? PS15, Line 197: // Identify the instance that is the most up-to-date and check that there : // are no duplicate UUIDs among the healthy input instances. Should add that the old invariant was that all instances have to have identical PathSetPBs. We're relaxing that invariant now and adding timestamps to PathSetPBs to identify the most up-to-date one. So if we find ourselves without any timestamps, it's OK to pick a path_set arbitrarily to use as the "main instance". PS15, Line 205: max_timestamp Nit: add the units (max_timestamp_us?). Line 208: const PathSetPB path_set = instances[i]->metadata()->path_set(); Store a cref; no need to make a copy. Line 218: updated_instances->insert(instances[i]); Can we modify a temporary set first, so that if we return an error we avoid touching 'updated_instances'? Line 231: int path_set_uuids_size; Isn't this always the same as main_uuids.size() after L232-244? Ah, I see, you're protecting against duplicates on L247-257. Maybe add a comment here to explain? Line 232: if (main_path_set->all_paths_size() == 0) { It's rather easy to forget that this means "legacy". Perhaps store it in an intermediate "bool is_legacy" or some such? PS15, Line 234: main_uuids.insert(main_path_set->deprecated_all_uuids().begin(), : main_path_set->deprecated_all_uuids().end()); : path_set_uuids_size = main_path_set->deprecated_all_uuids_size(); Nit: flip the order so it's the same as L239-243. Line 240: DCHECK_GT(path_set_uuids_size, 0); I think this is trivially correct (by virtue of being in this else statement). Line 242: main_uuids.insert(path.uuid()); Should this InsertOrDie? Or can we expect duplicates? PS15, Line 265: depcreated deprecated PS15, Line 267: main_path_updated main_path_set_updated? PS15, Line 302: main_path_set->mutable_all_paths(i) How about using pb_at_i here (though make it a pointer, not a cref). PS15, Line 325: path path_set http://gerrit.cloudera.org:8080/#/c/7270/15/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: PS15, Line 61: // Use log block mode. This should not have much effect on the directory : // manager itself, but log blocks are expected to be more common in production. : static const char* kBlockType = "log"; This is persisted (and compared) in PathInstanceMetadataFile, so it doesn't matter what value it is. May as well just pass "test" or something in the one place where you use it. PS15, Line 94: vector<string> GetDirNames(int num_dirs) { : vector<string> ret; : for (int i = 0; i < num_dirs; i++) { : string dir_name = Substitute("$0-$1", kDirNamePrefix, i); : ret.push_back(GetTestPath(dir_name)); : CHECK_OK(env_->CreateDir(ret[i])); : } : return ret; : } Don't need this anymore. Line 367: class DataDirManagerTest : public DataDirsTest { Is this class hierarchy absolutely necessary? I'd rather have one fixture total whose members aren't used in every test than a set of classes whose hierarchy is tougher to understand. Line 399: FLAGS_env_inject_eio = 1.0; Does this need to be reset at the end? Or is it OK because of eio_globs? http://gerrit.cloudera.org:8080/#/c/7270/6/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 796: std::lock_guard<percpu_rwlock> lock(dir_group_lock_); > Would you recommend a different kind of lock here? Yes, a sleeping mutex would be more appropriate. Though in an ideal world we wouldn't hold any locks during I/O; not sure if that can be done here. http://gerrit.cloudera.org:8080/#/c/7270/15/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: Line 159: void SetFailed(); Doc. Line 394: // Synchronizes the in-memory path sets to match the currently failed dirs. What's the significance of the return value? http://gerrit.cloudera.org:8080/#/c/7270/15/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: Line 356: LOG(INFO) << root; Still want this? http://gerrit.cloudera.org:8080/#/c/7270/15/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: Line 1287: ASSERT_OK(env_util::CreateDirIfMissing(env_, dir)); CreateDir() should suffice. http://gerrit.cloudera.org:8080/#/c/7270/15/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS15, Line 1537: // Blocks should not be placed in failed directories. : limit = 0; I don't think this has the effect you want. If we use a limit of 0, we'll still end up creating containers on disk; we just won't fill them up. PS15, Line 1619: FindDataDirByUuidIndex(uuid_indices[i]) Isn't this the same iteration order as dd_manager_->data_dirs()? That is, couldn't we replace this with dd_manager->data_dirs()[i]->dir()? -- To view, visit http://gerrit.cloudera.org:8080/7270 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifddf0817fe1a82044077f5544c400c88de20769f Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
