Adar Dembo has posted comments on this change. Change subject: disk failure: add persistent disk states ......................................................................
Patch Set 26: (17 comments) http://gerrit.cloudera.org:8080/#/c/7270/26/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: Line 36 How come we don't need this? http://gerrit.cloudera.org:8080/#/c/7270/26/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 422: Status DataDirManager::Open() { This could stand to be decomposed. It's so long and there's no clear description of what it's trying to accomplish, why in this particular order, etc. PS26, Line 540: vector<PathInstanceMetadataFile*> instances; : instances.resize(path_set.all_paths_size()); I think there's a constructor that resizes. Line 543: vector<DataDir*> failed_dirs; This is confusing w.r.t. failed_data_dirs. PS26, Line 587: path_set.all_paths(uuid_idx) Capture this in a local cref? PS26, Line 605: path_set_.Swap(&path_set); : auto path_set_reset = MakeScopedCleanup([&] { : path_set_.Swap(&path_set); : }); This is strange. Can't you parameterize the path set for UpdateInstanceFiles, then do the swap where all the other swaps go? PS26, Line 633: // Initialize the 'fullness' status of the data directories. : for (const auto& dd : data_dirs_) { : uint16_t uuid_idx; : CHECK(FindUuidIndexByDataDir(dd.get(), &uuid_idx)); : if (ContainsKey(failed_data_dirs_, uuid_idx)) { : continue; : } : Status refresh_status = dd->RefreshIsFull(DataDir::RefreshMode::ALWAYS); : if (PREDICT_FALSE(!refresh_status.ok())) { : if (refresh_status.IsDiskFailure()) { : MarkDataDirFailed(uuid_idx, refresh_status.ToString()); : continue; : } : return refresh_status; : } : } Why was this moved all the way down here? You could use instance health to determine if a data directory is healthy, no? Or is the issue that MarkDataDirFailed() won't work until all that stuff above is initialized? Line 662: string healthy_path = path_set_.all_paths(uuid_idx).path(); Not used? Line 670: set<int> indices_to_update(std::move(updated_uuid_indices)); Why not just operate on updated_uuid_indices directly? It was passed by value so it's a copy. PS26, Line 689: indices_to_update.erase(idx_to_update); Seems like you could do this right at .begin() and avoid duplicating it. Line 701: return Status::IOError("Could not write disk states, all disks failed"); Is there a test case for this? Looks like an interesting edge case. Line 744: // A size of 0 would indicate no healthy disks, which should crash the server. Why crash and not return an IOError, letting a caller higher in the stack crash? Line 908: CHECK(!read_only_) << "Cannot handle disk failures; filesystem is " Hmm, what if we just skipped WritePathHealthStates? Would that be incorrect? Line 909: "opened in read-only mode"; Nit: funky indentation here. Line 917: CHECK_NE(failed_data_dirs_.size(), data_dirs_.size()) << "All data dirs have failed"; Also surprised to see this here; why not let the higher levels (maintenance manager, etc.) discover this and decide to crash or not? Line 953: std::lock_guard<Mutex> lock(write_instance_mutex_); Can you add a comment explaining what this is serializing and why? http://gerrit.cloudera.org:8080/#/c/7270/26/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS26, Line 2183: // Note: disk failures when opening the block manager need only mark the : // directory as failed, rather than running callbacks to shut down tablets. At : // this point, the tablet manager is not initialized, and no such callback : // exists. This is pretty fragile; what if Repair() is refactored and some of the logic is used at runtime? How will we know that we need to change the macros to call the error manager's callback? Perhaps at startup the block managers should install a callback that fails data dirs, and later on, the tablet manager will replace it with one that fails entire tablets? -- 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: 26 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
