Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs ...................................................................... Patch Set 7: Code-Review+2 (5 comments) LGTM, just a few nits. http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/client/client-test.cc@5765 PS7, Line 5765: ASSERT_OK(bad_guy_scanner.Open()); nit: does it make sense to test the behavior of the KuduScanner::Open() method in the case when the scanner tries to pre-fetch some rows (i.e. set the initial batch size to non-zero and see how it works)? http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/scanners.cc File src/kudu/tserver/scanners.cc: http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/scanners.cc@168 PS7, Line 168: if (username != ret->remote_user().username()) { : *error_code = TabletServerErrorPB::NOT_AUTHORIZED; : return Status::NotAuthorized(Substitute("User $0 doesn't own scanner $1", : username, scanner_id)); : } nit: do we prefer exposing the fact that the scanner exists but the caller is not authorized to access it versus reporting NotFound error and logging into the log about non-authorized attempt to access the scanner? http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/scanners.cc@173 PS7, Line 173: ret nit: wrap into std::move() ? http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/tablet_server-test.cc@3406 PS7, Line 3406: it nit: them ? http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/tablet_server-test.cc@3437 PS7, Line 3437: const nit: might it be constexpr? -- To view, visit http://gerrit.cloudera.org:8080/6348 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4 Gerrit-Change-Number: 6348 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 06 Nov 2018 02:56:41 +0000 Gerrit-HasComments: Yes
