Abhishek Chennaka 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) 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 Done http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/fs/data_dirs.cc@85 PS5, Line 85: the block manager. > nit: WAL directory Done 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 Done 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. Alm Done 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 re Done 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 Done http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@437 PS5, Line 437: int64 > nit: int64_t? Done 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 Done 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...) Done 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 wo Done 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 de Yes, tested that in my local mac machine earlier by adding a new volume. Snippet of df -h: Filesystem 512-blocks Used Available Capacity iused ifree %iused Mounted on /dev/disk1s1 976490576 21607536 654724040 4% 488003 4881964877 0% / /dev/disk1s6 466Gi 696Ki 312Gi 1% 72 4882452808 0% /Volumes/Test In both the below data directories the space usage reported is identical: Test 1: /Users/achennaka/master-0/data,/Volumes/Test/kudu,/Volumes/Test/kudu1 Test 2: /Users/achennaka/master-0/data,/Volumes/Test/kudu -- 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: Sat, 14 Aug 2021 06:22:18 +0000 Gerrit-HasComments: Yes
