Zoltan Martonka 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 Done 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? Done 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 Done 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 Done 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 returning status now -- 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-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Wed, 19 Jun 2024 11:54:12 +0000 Gerrit-HasComments: Yes
