Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/17725/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17725/3//COMMIT_MSG@9 PS3, Line 9: This commit adds two metrics to server entity. : wal_dir_space_available_bytes - Exposes the free space available in the : wal directory : data_dirs_space_available_bytes - Exposes the sum of free space in each : of the data directories. nit: may be more readable as This commit adds two metrics to server entity. - wal_dir_space_available_bytes: Exposes the free space available in the WAL directory. - data_dirs_space_available_bytes: Exposes the sum of free space in each of the data directories. http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/fs/fs_manager.cc@101 PS3, Line 101: DEFINE_int64(fs_wal_dir_reserved_bytes, -1, Flags should only be defined once, so we should remove the definition in the consensus module. http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@258 PS3, Line 258: wal nit: capitalized WAL here and below http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@431 PS3, Line 431: con nit: add a space http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@431 PS3, Line 431: int64_t GetWalDirAvailableSpace(const ServerBaseOptions options,const FsManager& fs_manager) { We should pass the options as a const reference, otherwise we'll be making a copy of the options every time this is called. Same below. http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@434 PS3, Line 434: if (!options.env->GetSpaceInfo(fs_manager.GetWalsRootDir(), &space_info_waldir).ok()) { : return -1; : } This seems a bit surprising to see as a metric. I'm curious, as a user whether it would be more useful to cache the current state of this metric and return a stale value if this fails. If we do that, we could also introduce some form of rate limiting a la --fs_data_dirs_available_space_cache_seconds, so we're not exposing a system call every time we want to get this metric (even if we only expect it to infrequent). http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@452 PS3, Line 452: unique_fsid nit: unique is implied because this is a set. Maybe just call this 'fs_ids'? http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@453 PS3, Line 453: const string & nit: put the ampersand next to "string", i.e. "const string& data_dir", or better yet, "const auto& data_dir" http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@457 PS3, Line 457: (FLAGS_fs_data_dirs_reserved_bytes == -1) nit: we could define this once up front as a bool as some bool should_reserve_one_percent = FLAGS_fs_data_dirs_reserved_bytes == -1; That also has the benefit of being somewhat self-explanatory. http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@460 PS3, Line 460: if (space_info_datadir.free_bytes > reserved_bytes) { : total_free_space += space_info_datadir.free_bytes - reserved_bytes; : } nit: would be simpler as total_free_space += max(0, space_info_datadir.free_bytes - reserved_bytes); http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@926 PS3, Line 926: [this]() {return GetDataDirAvailableSpace(options_, *fs_manager_.get());})-> > warning: redundant get() call on smart pointer [readability-redundant-smart Right, you can just do *fs_manager_ to pass as a reference -- that's true for all smart pointer types. -- To view, visit http://gerrit.cloudera.org:8080/17725 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0 Gerrit-Change-Number: 17725 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 30 Jul 2021 04:09:31 +0000 Gerrit-HasComments: Yes
