Andrew Wong 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 7: (7 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: > This could be &dds[i] to avoid the local 'dd'. Done 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)? Done 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: // DataDirGroup : //////////////////////////////////////////////////////////// : : DataDirGroup::DataDirGroup() {} : > Could we push this down into VerifySufficientDiskSpace? That'd perhaps make I don't see a very nice way to reuse ShouldInject without putting ShouldInject into env.h or moving VerifySufficientDiskSpace into env_posix. Both don't seem great. For now, I'll just move these definitions into env_util.cc with VerifySufficientDiskSpace. http://gerrit.cloudera.org:8080/#/c/14122/5/src/kudu/fs/data_dirs.cc@1099 PS5, Line 1099: > Maybe defer this copy until L1106, where it's actually needed? Done http://gerrit.cloudera.org:8080/#/c/14122/5/src/kudu/fs/data_dirs.cc@1101 PS5, Line 1101: > has? Done http://gerrit.cloudera.org:8080/#/c/14122/5/src/kudu/fs/data_dirs.cc@1102 PS5, Line 1102: DCHECK(dir_group_lock_.is_locked()); > Is it possible for new_target_group_size to be _less than_ group_uuid_indic No, because we only ever target one additional directory, and because we no-op if we see that we're at the target size. It's not hard to protect against that anyway, so I'll add it here. http://gerrit.cloudera.org:8080/#/c/14122/5/src/kudu/fs/data_dirs.cc@1129 PS5, Line 1129: int tablets_in_second = FindOrDie(tablets_by_uuid_idx_map_, candidate_indices[1]).size(); : int s > Consider putting this ahead of the add-new-directory case so as to avoid th Done -- 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: 7 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 06:54:56 +0000 Gerrit-HasComments: Yes
