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 5: (11 comments) Mostly nits here. Looking pretty good otherwise! http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/fs/data_dirs.cc@80 PS5, Line 80: nit: not related to this change, but there's an extra space at the end here http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/fs/data_dirs.cc@85 PS5, Line 85: the block manager. nit: WAL directory http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/integration-tests/disk_failure-itest.cc File src/kudu/integration-tests/disk_failure-itest.cc: http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/integration-tests/disk_failure-itest.cc@336 PS5, Line 336: nit: remove the extra space. Same at L358 http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/integration-tests/disk_failure-itest.cc@336 PS5, Line 336: ASSERT_NE(wal_dir_space, -1 ); : ASSERT_NE(data_dir_space, -1); Could we instead ASSERT_GT(wal_dir_space, 0)? Same with data_dir_space. Almost surely the test machines will have space. Same at L348? http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/integration-tests/disk_failure-itest.cc@349 PS5, Line 349: LOG(INFO) << "Restarting server with injected errors..."; nit: this is useful while iterating on a test, but before merging, let's remove the extra log, given if we needed to debug a test failure, it should be clear when we restart the server without this line. http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@262 PS5, Line 262: + nit: remove the "+"? Same at L268 http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@437 PS5, Line 437: int64 nit: int64_t? http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@468 PS5, Line 468: data_dirs_available_space nit: wrap variable names with single quotes so it's easier upon reading to immediately tell that this is referring to a variable. Same elsewhere. http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@472 PS5, Line 472: fs_data_dirs_available_space_cache_seconds nit: prefix with -- (i.e. --fs_data_di...) http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@961 PS5, Line 961: [this]() {GetDataDirAvailableSpace(options_, *fs_manager_, : &data_dirs_space_last_check_, &data_dirs_available_space_); return data_dirs_available_space_;})-> : AutoDetachToLastValue(&metric_detacher_); style nit: typically we style lambdas (especially multi-line ones) as we would regular functions. METRIC_...InstantiateFunctionGauge( metric_entity_, [this]() { GetDataDirAvailableSpace(options_, *fs_manager_, &data_dirs_space_last_check_, &data_dirs_available_space_); return data_dirs_available_space_; }) ->AutoDetachToLastValue(&metric_detatcher_); http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/util/env_posix.cc@1446 PS5, Line 1446: space_info->filesystem_id = buf.f_fsid; I'm curious if you've tried this on a real machine with multiple mounted devices. In tests it's a bit difficult to check that, but it's probably worth manually verifying. E.g. maybe try mounting two data directories in one volume, and one data directory in a separate one. We should see that the available space should be the sum available in the two volumes (no more, no less). If you do end up performing that manual test, be sure to mention what you did in the commit message. -- 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: 5 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, 13 Aug 2021 01:04:09 +0000 Gerrit-HasComments: Yes
