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: (10 comments) http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@446 PS2, Line 446: return std::max(static_cast<int64_t> (0), space_info.free_bytes - reserved_bytes); > Not exactly, I meant wrapping the calculation too, to make sure they're cal Done http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@259 PS4, Line 259: erv > nit: WAL. Same elsewhere Done http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@266 PS4, Line 266: "data directories space free", > We should also mention the error behavior that this returns -1 Done http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@431 PS4, Line 431: return shared_ptr<MemTracker>(MemTracker::C > nit: could you also document the error semantics? Same with the other metho Done http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@432 PS4, Line 432: } > nit: this doesn't need to have a return value if values are being returned Done http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@434 PS4, Line 434: WAL/data directory's di > nit: we should keep the naming of our variables standardized as separating Done http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@434 PS4, Line 434: the given WAL/data directory's disk. Returns -1 in case of disk : // failure. > nit: parameters that are output values (even if they're also input values) Done http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@437 PS4, Line 437: int64 flag_reserved_b > As discussed with Andrew offline will add a new flag. Done http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@443 PS4, Line 443: should_reserve_one_percent = flag_reserved_bytes == -1; : reserved_bytes = should_reserve_one > Yes, that makes sense. Will try to ensure: As disucssed it is designed to: 1. For WAL dir failure initialize it to -1 and return -1 in case of disk failure after the TS started 2. For data dir initialize it to -1 and return -1 if any of the calls to statvfs fails. So in case of complete initial failure of WAL or all data disks, -1 will be returned. http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@447 PS4, Line 447: } > should_preserve_one_percent is already a boolean. You don't need further co 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: 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: Tue, 10 Aug 2021 18:58:38 +0000 Gerrit-HasComments: Yes
