ZhangYao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13975 )
Change subject: Consider the available space when selecting data dirs for blocks. ...................................................................... Patch Set 8: (9 comments) http://gerrit.cloudera.org:8080/#/c/13975/7/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: http://gerrit.cloudera.org:8080/#/c/13975/7/src/kudu/fs/data_dirs.h@207 PS7, Line 207: ailable_bytes_. > nit: wrap this in ''s like the other variable names. Done http://gerrit.cloudera.org:8080/#/c/13975/7/src/kudu/fs/data_dirs.h@212 PS7, Line 212: , updated by RefreshAvailab > nit: remove this, or update it to RefreshAvailableSpace Done http://gerrit.cloudera.org:8080/#/c/13975/7/src/kudu/fs/data_dirs.h@338 PS7, Line 338: and gives preference to those with more free : // space in the specified option's data dir gr > nit: Could you rephrase a bit? Done http://gerrit.cloudera.org:8080/#/c/13975/7/src/kudu/fs/data_dirs.h@471 PS7, Line 471: as the number of unique tablets in the directory. : // If those two directories have same load, will prefer the one with more : // free space. The resulting behavior fills directories that hav > nit: update this? Done http://gerrit.cloudera.org:8080/#/c/13975/7/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/13975/7/src/kudu/fs/data_dirs.cc@90 PS7, Line 90: "); : TAG_FLAG(fs_data_dirs_available_space_cache_seconds, advanced); > nit: I would remove this sentence. These descriptions usually try to be use Done http://gerrit.cloudera.org:8080/#/c/13975/7/src/kudu/fs/data_dirs.cc@110 PS7, Line 110: DECLARE_bool(enable_data_block_fsync); : DECLARE_string(block_manager); : > nit: can you remove this extraneous newline? Done http://gerrit.cloudera.org:8080/#/c/13975/7/src/kudu/fs/data_dirs.cc@264 PS7, Line 264: > nit: maybe we should rename this to last_space_check_ or something similar Done http://gerrit.cloudera.org:8080/#/c/13975/7/src/kudu/fs/data_dirs.cc@1002 PS7, Line 1002: candidate_indices[0]); : DataDir* candidate_second = FindOrDie(data_dir_by_uu > Could we just shuffle candidate_indices directly? And then compare dirs can Of course :) http://gerrit.cloudera.org:8080/#/c/13975/7/src/kudu/fs/data_dirs.cc@1083 PS7, Line 1083: int64_t space_in_second = FindOrDie(data_dir_by_uuid_idx_, : candidate_indices[1])->available_bytes(); : selected_index = space_in_first > space_in_second ? 0 : 1; : } else { : selected_index = tablets_in_first < tablets_in_second ? 0 : 1; : } : group_indices->push_back(candidate_indices[selected_index]); : candidate_indices.erase(candidate_indices.begin() + selected_index); : } : } : } : : DataDir* DataDirManager::FindDataDirByUuidIndex(int uuid_idx) const { : DCHECK_LT(uuid_idx, data_dirs_.size()); : ret > To be clear, this is selecting by the tablet count first, and then breaking If we select by space first, it may assign a lot of tablets in the directory with more space when creating a lot of tablets at once and doesn't fill up so quick just like what Adar said. But we have PO2C, it may not happen. But I still afraid of that :( -- To view, visit http://gerrit.cloudera.org:8080/13975 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I194c4965ee64aed728e3b84e684c04d445cbe529 Gerrit-Change-Number: 13975 Gerrit-PatchSet: 8 Gerrit-Owner: ZhangYao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: ZhangYao <[email protected]> Gerrit-Comment-Date: Fri, 09 Aug 2019 03:34:00 +0000 Gerrit-HasComments: Yes
