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 10: (18 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 ? Done http://gerrit.cloudera.org:8080/#/c/17725/10//COMMIT_MSG@24 PS10, Line 24: directores > nit: directories Done 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 alway Done. Applied the validator for --fs_data_dirs_available_space_cache_seconds (above) too. http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/integration-tests/disk_failure-itest.cc File src/kudu/integration-tests/disk_failure-itest.cc: http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/integration-tests/disk_failure-itest.cc@352 PS10, Line 352: E > nit: got a couple extra spaces here Done http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/integration-tests/disk_failure-itest.cc@364 PS10, Line 364: ASSERT_OK(itest::GetInt64Metric(cluster_->tablet_server(0)->bound_http_hostport(), : &METRIC_ENTITY_server, nullptr, &METRIC_wal_dir_space_available_bytes, "value", : &wal_dir_space)); : ASSERT_OK(itest::GetInt64Metric(cluster_->tablet_server(0)->bound_http_hostport(), : &METRIC_ENTITY_server, nullptr, &METRIC_data_dirs_space_available_bytes, "value", : &data_dir_space)); : ASSERT_NE(wal_dir_space, -1); > nit: we can use get_metrics() here too, right? Done 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 Done 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 Done 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" Done 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: Done 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 d Done 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 ? Yes, it was 0LL in patchset 4. But I remember one of the test configurations (maybe ASAN or TSAN) was not happy about it, hence changed it to the above. Do not have the old logs anymore when checked. I'll resubmit the next patch with 0LL and we can check it. 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 p Yes, that makes sense. Done. 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 Done 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 Done 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? Done 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? Done 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 inten We make data_dirs_available_space to point to zero in line 484 just before the 'for()' 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? Oops. Done -- 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: Mon, 23 Aug 2021 15:26:51 +0000 Gerrit-HasComments: Yes
