Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs ...................................................................... Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test-base.h File src/kudu/tserver/tablet_server-test-base.h: http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test-base.h@125 PS3, Line 125: Status FillNewScanRequest(ReadMode read_mode, NewScanRequestPB* scan) const; > Could it be a static method (or at least const)? Done, const since it's using `schema_` http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc@3400 PS3, Line 3400: // bytes should be read by the BM if storing keys in the rowset metadata). > Is it worth adding a higher-level test to make sure this works as expected? I'll keep this in mind for further authz testing. http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc@3407 PS3, Line 3407: uy"); > Does it make sense to add a scenario to verify that without user_credential Done http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_service.cc@1292 PS3, Line 1292: void TabletServiceImpl::ScannerKeepAlive(const ScannerKeepAliveRequestPB *req, > I'd suggest going ahead and doing this as a part of this change, unless it' Done http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_service.cc@2066 PS3, Line 2066: &scanner); > Why not populate *error_code with something more detailed? Is UNKNOWN_ERROR Done http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tserver_path_handlers.cc File src/kudu/tserver/tserver_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tserver_path_handlers.cc@540 PS3, Line 540: json->Set("requestor", scan.remote_user.username()); > Also add an underscore, I think that's the prevailing style for our JSON ke Hrm, I think I'll keep requestor, even if it isn't exactly consistent; in the user-facing context of a scan, it's clear what a Requestor is. -- 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: 4 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Nov 2018 22:17:11 +0000 Gerrit-HasComments: Yes
