Todd Lipcon has posted comments on this change. Change subject: disk failure: make DataDirManager failure-aware ......................................................................
Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/7028/6/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: PS6, Line 222: are not the in the failed typo http://gerrit.cloudera.org:8080/#/c/7028/6/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: PS6, Line 459: uint32_t group_target_size would this be simpler to just be an 'int'? GSG discourages using uints for things that we expect to just be small integer counts PS6, Line 632: LOG(ERROR) << Substitute("Dir with uuid index $0 ($1) marked as failed", uuid_idx, dd->dir()); Would it be convenient to take a Status or a string message as an argument to this function that could be used to improve this log message? Would be nice if, when we saw this, we could see some indication of the underlying issue which caused it to be marked failed (eg out of space, IO error, readonly FS, etc) http://gerrit.cloudera.org:8080/#/c/7028/6/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS6, Line 268: served to or from nit: reads a bit funny PS6, Line 280: // Returns true if all data dirs registered with the DataDirManager are healthy. : // If there are failed data dirs, populates 'healthy_indices' with those in : // 'uuid_indices' that have not failed and returns false. hrm, this is a sort of odd contract... if I understand correctly, you're saying that if all are healthy, then healthy_indices won't be touched, but if some are unhealthy, then healthy_indices will contain the healthy ones? What if there is an unhealthy disk that is not listed in 'uuid_indices'? Would it return true or false? Is there a simpler API that could be used here, like: void RemoveUnhealthyDirs(vector<uint16_t>* uuid_indices)? Line 304: // lock_guard of both dir_group_lock_ and dir_health_lock_. I don't see dir_health_lock_. Is this left from a previous iteration of the patch? -- To view, visit http://gerrit.cloudera.org:8080/7028 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2 Gerrit-PatchSet: 6 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: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
