Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21509 )
Change subject: Fix cache cleaning in dense_node-itest ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/21509/1/src/kudu/integration-tests/dense_node-itest.cc File src/kudu/integration-tests/dense_node-itest.cc: http://gerrit.cloudera.org:8080/#/c/21509/1/src/kudu/integration-tests/dense_node-itest.cc@251 PS1, Line 251: kudu:: nit: this call site is already in kudu namespace, so it should be possible to drop 'kudu::' prefix here? http://gerrit.cloudera.org:8080/#/c/21509/1/src/kudu/util/os-util.cc File src/kudu/util/os-util.cc: http://gerrit.cloudera.org:8080/#/c/21509/1/src/kudu/util/os-util.cc@127 PS1, Line 127: file_name nit: is this actually path to the file, not just name? http://gerrit.cloudera.org:8080/#/c/21509/1/src/kudu/util/os-util.cc@127 PS1, Line 127: int Consider an alternative returning 'Status' instead of just 'int': with that, it's possible to include information on errno, propagating that from the syscalls. You could see PosixWritableFile::Close(), SyncDir::SyncDir(), etc. in env_posix.cc for reference. http://gerrit.cloudera.org:8080/#/c/21509/1/src/kudu/util/os-util.cc@134 PS1, Line 134: write_ret < -1 I'm not sure this does what it's supposed to do: on error, write() returns -1, at least per https://man7.org/linux/man-pages/man2/write.2.html http://gerrit.cloudera.org:8080/#/c/21509/1/src/kudu/util/os-util.cc@142 PS1, Line 142: if (close_ret < 0) : return close_ret; : return 0; nit: could this be just return close_ret; ? -- To view, visit http://gerrit.cloudera.org:8080/21509 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f36f5a97d9a032aeb495989b4dc05191bf66425 Gerrit-Change-Number: 21509 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Martonka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 13 Jun 2024 23:56:51 +0000 Gerrit-HasComments: Yes
