Andrew Wong has posted comments on this change. Change subject: disk failure: add persistent disk states ......................................................................
Patch Set 6: (29 comments) http://gerrit.cloudera.org:8080/#/c/7270/6/src/kudu/fs/block_manager_util-test.cc File src/kudu/fs/block_manager_util-test.cc: PS6, Line 65: ASSERT_EQ(kUuid, path_set.all_paths(0).uuid()); : ASSERT_EQ(PathHealthStatePB::HEALTHY, path_set.all_paths(0).health_state()); : ASSERT_EQ(kPath, path_set.all_paths(0).path()); > Nit: maybe extract a "const PathPB& path" so you're not repeating path_set. Done http://gerrit.cloudera.org:8080/#/c/7270/6/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: Line 53: healthy_instance_ = false; \ > Maybe we should store the Status that made us unhealthy instead? Then we co Done Line 88: // Set up the path set. > Should we still populate all_uuids, to support downgrades? Or does that not Good point, that should work. Line 91: new_path_set->set_timestamp_us(1); > Shouldn't this be a value of now? It shouldn't matter at this point since these are all in memory. The timestamps indicate when the path set was last written and are updated right before they're written to disk. Since they haven't been written yet, an arbitrarily low value here should be fine. PS6, Line 97: new_path_set->add_all_paths(); : *new_path_set->mutable_all_paths(i) = new_path; > Can you combine: Done Line 149: Status PathInstanceMetadataFile::Lock() { > Do we expect disk failure errors in LockFile or UnlockFile? Should we use R Done http://gerrit.cloudera.org:8080/#/c/7270/6/src/kudu/fs/block_manager_util.h File src/kudu/fs/block_manager_util.h: PS6, Line 109: PathInstanceMetadataPB* > Pass by cref? Ah, here, pb's timestamp gets updated to the current time. Will update the comment. Line 116: bool healthy_instance_; > Nit: how about just healthy_ (and healthy() for the accessor)? Done http://gerrit.cloudera.org:8080/#/c/7270/6/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 212: if (instance()->healthy_instance()) { > A silent no-op if the instance isn't healthy seems error-prone. Can we mak Done Line 256: metadata_file_->SetInstanceFailed(); > This is weird; it's the DataDir that failed, not the metadata file. Why not Right. Right now, failures of RefreshIsFull() are handled externally, because handling the failure of a data dir needs coordination of the DataDirManager's locks, so this shouldn't be in this function at all. I'll make this more consistent. Line 359: uuid_by_path_.swap(uuid_by_path); > Why do this before SyncDir()? Shouldn't it be safer to do after we're guara Done PS6, Line 406: if (!new_status.IsDiskFailure()) { : return new_status; : } > Oh, so I think I get this. The reason behind it was that I'd originally wanted the instance to only be considered "failed" if there was issues creating the instance's members, since the path_set could be in some messed up state. Only after did I realized that we _should_ handle them anyway because the presence of a disk failure is information to be used by, say, CheckIntegrity. I've updated this to be PathInstanceMetadataFile-internal. Line 419: if (PREDICT_TRUE(instance->healthy_instance())) { > Why not create the pool either way? It's very low overhead and will mean yo Done Line 450: dd->mutable_instance()->SetInstanceFailed(); > Here's another case where the PathInstanceMetadataFile disk failure is dete I opted to keep the behavior where instance files know about their failure. PS6, Line 460: &path_set_ > Let's not make changes to member state until we can't fail anymore. Done PS6, Line 472: return Status::IOError(Substitute("Need to write $0 new instance files but filesystem opened " : "in read-only mode", updated_indices.size())); > Why is this fatal? If we're in read-only mode, can't we move forward despit Done PS6, Line 476: ; > Nit: extra semicolon Done Line 487: dd->WaitOnClosures(); > This should be safe regardless of dd health. Done Line 677: Status s = candidate->RefreshIsFull(DataDir::RefreshMode::EXPIRED_ONLY); > Likewise. Done Line 724: Status s = e.second->RefreshIsFull(DataDir::RefreshMode::ALWAYS); > Why aren't we returning RefreshIsFull errors anymore? Done Line 771: CHECK(AccessMode::READ_WRITE == mode_) << "Cannot handle disk failures; filesystem is " > Use CHECK_EQ. Done Line 796: void DataDirManager::WritePathHealthStatesUnlocked() { > Should DCHECK on the lock that needs to be held. Would you recommend a different kind of lock here? http://gerrit.cloudera.org:8080/#/c/7270/6/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS6, Line 339: dirty_idx_list > you mean 'instances'? Done PS6, Line 343: // This only affects the DataDirManager state, so it shouldn't be used if : // state out of its jurisdiction (e.g. tablets) needs to be updated. > Not exactly sure what guidance this provides. Done Line 352: const AccessMode mode_; > Nit: group with the other const members (i.e. remove the blank line separat Done http://gerrit.cloudera.org:8080/#/c/7270/6/src/kudu/fs/fs.proto File src/kudu/fs/fs.proto: Line 70: repeated bytes all_uuids = 2; // Deprecated field used before Kudu 1.5.0. > Another convention is to rename deprecated proto fields by prepending "DEPR Done PS6, Line 75: There is no PathSetPB with a UUID outside the set of UUIDs in : // 'all_paths' of the PathSetPB with the highest 'timestamp_us' > This is a pretty dense sentence and is difficult to understand. Reword? Done http://gerrit.cloudera.org:8080/#/c/7270/6/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 1493: continue; > Won't this lead to a crash when we look up this dd's limit in block_limits_ Done. LogBlockContainers shouldn't be placed in failed directories, so I set this limit to 0. Line 1538: continue; > This won't increment 'i'; are we missing some test coverage here? Seems lik Ah, didn't add test coverage. Will update in the next iteration. -- 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: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
