ZhangYao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13975 )
Change subject: KUDU-2901: Consider the available space when selecting data dirs for tablets and blocks. ...................................................................... Patch Set 11: (13 comments) http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/fs/data_dirs.h@207 PS10, Line 207: ilable_bytes_. > // Protects 'last_space_check_', 'is_full_' and 'available_bytes_'. Done http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/fs/data_dirs.h@338 PS10, Line 338: e one > the one ? Done http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/fs/data_dirs.h@338 PS10, Line 338: // Returns a random directory, giving preference to the one with more free > Nit: "...directory, giving preference to..." Done http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/fs/data_dirs.h@340 PS10, Line 340: , > nit: keep the blank space. Done http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/fs/data_dirs.h@472 PS10, Line 472: // Ties are broken by choosing the directory with more free space. The : // resulting be > Nit: Rephrase as "Ties are broken by choosing the directory with more free Done http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/fs/data_dirs.cc@90 PS10, Line 90: "Number of seconds we cache the available disk space in the block manager. "); > Nit: "Number of seconds we cache available disk space in the block manager. Done http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/fs/data_dirs.cc@261 PS10, Line 261: last_space_check_ > How about change it to expiry meaning, then we don't need to add FLAGS_fs_d If change it to expiry meaning, it will need to add FLAGS_fs_data_dirs_available_space_cache_seconds in ALWAYS mode. Maybe last_space_check_ is clear enough and what's more it can work when FLAGS_fs_data_dirs_available_space_cache_seconds is change by unit test during expire time. http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/fs/data_dirs.cc@269 PS10, Line 269: int64_t available_bytes_new; > Nit: keeping in with "is_full_" and "is_full_new", perhaps name this "avail Done http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/fs/data_dirs.cc@984 PS10, Line 984: vector<DataDir*> candidate_dirs; > Wouldn't this be simpler as a vector<DataDir*>? Seems like we could avoid a Yeah. Done. http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/fs/data_dirs.cc@999 PS10, Line 999: if (candidate_dirs.size() >= 2) { > This can be moved into the if block to avoid a no-op call when candidate_in Done http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/fs/data_dirs.cc@1072 PS10, Line 1072: } else { > Seems some redundant with L994~L1006, how about do some refactor? Yeah:) And after this patch, above L994~L1006 was changed. Maybe there is no need to refactor since they are different? http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/util/env_util.h File src/kudu/util/env_util.h: http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/util/env_util.h@56 PS10, Line 56: // If 'available_bytes' is not null, it will contain the amount of free disk space (in bytes) > Nit: "If 'available_bytes' is not null, it will contain the amount of free Done http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/util/env_util.cc File src/kudu/util/env_util.cc: http://gerrit.cloudera.org:8080/#/c/13975/10/src/kudu/util/env_util.cc@159 PS10, Line 159: "($2 bytes available vs $3 bytes reserved)", : requested_bytes, path, free_bytes, reserved_bytes), : > Don't we want this to happen even if we return an ENOSPC IOError on L154? O Yeah... You are right. I originally put it here because if there is an IOERROR, this dir will not be selected when creating tablets, so its available_bytes will not be used. I think it would cause bug because is_full is judged before available_bytes and creating tablets(using ALWAYS) is before creating block(using EXPIRED). So this garbage available_bytes have no chance to be used. However, I think it should update the available_bytes even if return IOError for the functional semantic integrity. -- 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: 11 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: Tue, 13 Aug 2019 11:44:14 +0000 Gerrit-HasComments: Yes
