Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14122 )
Change subject: KUDU-2907: add a dir to dir groups when full ...................................................................... Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/14122/5/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: http://gerrit.cloudera.org:8080/#/c/14122/5/src/kudu/fs/data_dirs-test.cc@268 PS5, Line 268: dd This could be &dds[i] to avoid the local 'dd'. http://gerrit.cloudera.org:8080/#/c/14122/5/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: http://gerrit.cloudera.org:8080/#/c/14122/5/src/kudu/fs/data_dirs.h@339 PS5, Line 339: // 'opts'. If none exists, adds a new dir to the group and returns the dir. Want to add what happens if all remaining dirs are full (or if none exist)? Or is that too much detail? http://gerrit.cloudera.org:8080/#/c/14122/2/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/14122/2/src/kudu/fs/data_dirs.cc@1121 PS2, Line 1121: int new_uuid_idx = group_uuid_indices.back(); > Yes, this will happen when we flush the tablet metadata next (e.g. when a n Could you add a comment here explaining that somewhat? It's not weird to expect explicit superblock manipulation here to add the new group. http://gerrit.cloudera.org:8080/#/c/14122/5/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/14122/5/src/kudu/fs/data_dirs.cc@300 PS5, Line 300: bool inject_full = ShouldInjectSpaceError(dir_); : if (inject_full) { : available_bytes_new = 0; : s = Status::IOError(Env::kInjectedFailureStatusMsg, "", ENOSPC); : } Could we push this down into VerifySufficientDiskSpace? That'd perhaps make it easier to reuse ShouldInject from env_posix.cc. http://gerrit.cloudera.org:8080/#/c/14122/5/src/kudu/fs/data_dirs.cc@1099 PS5, Line 1099: vector<int> group_uuid_indices = group.uuid_indices(); Maybe defer this copy until L1106, where it's actually needed? http://gerrit.cloudera.org:8080/#/c/14122/5/src/kudu/fs/data_dirs.cc@1101 PS5, Line 1101: as has? http://gerrit.cloudera.org:8080/#/c/14122/5/src/kudu/fs/data_dirs.cc@1102 PS5, Line 1102: if (new_target_group_size == group_uuid_indices.size()) { Is it possible for new_target_group_size to be _less than_ group_uuid_indices.size()? Like, if two threads concurrently added new directories? http://gerrit.cloudera.org:8080/#/c/14122/5/src/kudu/fs/data_dirs.cc@1129 PS5, Line 1129: // If it's some other error, return it. : return s; Consider putting this ahead of the add-new-directory case so as to avoid the extra nesting. -- To view, visit http://gerrit.cloudera.org:8080/14122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic8e05bdfe1fc2ac0e7152d493f289d3ac6e850d7 Gerrit-Change-Number: 14122 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: ZhangYao <[email protected]> Gerrit-Comment-Date: Tue, 27 Aug 2019 04:15:47 +0000 Gerrit-HasComments: Yes
