Alexey Serbin 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 10: Verified+1 (16 comments) http://gerrit.cloudera.org:8080/#/c/17725/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17725/10//COMMIT_MSG@22 PS10, Line 22: mac nit: macOS ? http://gerrit.cloudera.org:8080/#/c/17725/10//COMMIT_MSG@24 PS10, Line 24: directores nit: directories http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/fs/data_dirs.cc@84 PS10, Line 84: fs_wal_dir_available_space_cache_seconds Is it intentional that setting this flag to a negative value leads to always stale value of the available WAL dir space metric? If not, maybe add a flag validator for this and the --fs_data_dirs_available_space_cache_seconds flag (something like we have for the --fs_wal_dir_reserved_bytes flag). http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@260 PS10, Line 260: WAL directory space free" nit: usually, the metric's label hash all words started with capital letter, something like "WAL Directory Free Space" http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@262 PS10, Line 262: Returns " : "-1 if reading the disk fails nit: maybe rephrase as Set to -1 if failed to retrieve disk metadata http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@266 PS10, Line 266: "data directories space free" nit: "Data Directories Free Space" http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@268 PS10, Line 268: Returns " : "-1 if reading any of the disks fails nit: Set to -1 if failed to retrieve metadata of any of the disks http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@438 PS10, Line 438: int64_t reserved_bytes; : bool should_reserve_one_percent; nit: move these down where they assigned with their values, turning these declarations into definitions? http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@446 PS10, Line 446: static_cast<int64_t> (0) nit: could replace with 0LL ? http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@456 PS10, Line 456: int64_t* wal_dir_available_space Why not to return this from this function? Why to keep this as an output parameter? http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@474 PS10, Line 474: indentation nit: remove one space here and for two lines below http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@476 PS10, Line 476: int64_t* data_dirs_available_space Why to have this as an output parameter? Maybe, just return the calculated value from this function? http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@479 PS10, Line 479: SpaceInfo space_info_datadir; nit: move this under the 'if()' scope below? http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@483 PS10, Line 483: int64_t available_space; nit: move this under the scope of the 'for()' cycle below? http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@494 PS10, Line 494: data_dirs_available_space What if data_dirs_available_space points to non-zeroed memory? Is it intentional just to add up to the value passed into this function? http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/util/env.h File src/kudu/util/env.h: http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/util/env.h@48 PS10, Line 48: q:q nit: drop these trailing symbols? -- 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: 10 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, 20 Aug 2021 02:04:48 +0000 Gerrit-HasComments: Yes
