Andrew Wong 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 Done 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 I don't think it makes things simpler since a cast is still needed for the *.size() call, but I'll switch out uint32_t for uniformity with GSG. 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 Done 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 Done 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 sa In that case, it would return false and use use the outputted healthy_indices. I'd wanted to piggy-back this function to also be a check emptiness of failed_data_dirs_. A cleaner API is probably more important here since the (has failed data dirs) branch should ideally not be used much anyway. 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 Yes and done. -- 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
