Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11753 )
Change subject: authz: verify tokens on scans ...................................................................... Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/11753/7/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/11753/7/src/kudu/common/schema.h@552 PS7, Line 552: std::vector<ColumnId> column_ids() const { Can you return a cref of col_ids_ instead? It'll make some operations (like a method that takes an iterator range and you want to feed in all of column_ids()) more efficient. http://gerrit.cloudera.org:8080/#/c/11753/7/src/kudu/tserver/tablet_server_authorization-test.cc File src/kudu/tserver/tablet_server_authorization-test.cc: http://gerrit.cloudera.org:8080/#/c/11753/7/src/kudu/tserver/tablet_server_authorization-test.cc@444 PS7, Line 444: const int kNumKeys = 5; : const int kNumVals = 5; : const char* kScanTableName = "scan-table-name"; : const char* kScanTabletId = "scan-tablet-id"; : const char* kScanUser = "good-guy"; All of these can be declared as static. And if Alexey is going to review this, I suggest you make them constexpr now and save yourself the trouble. :) http://gerrit.cloudera.org:8080/#/c/11753/7/src/kudu/tserver/tablet_server_authorization-test.cc@509 PS7, Line 509: pb.set_read_mode(READ_AT_SNAPSHOT); Why this? http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@427 PS4, Line 427: return false; > Done Regarding IS_DELETED, it's kind of murky. In theory you could add it to any projection or use it in a projection by itself, though that wouldn't be too useful unless your predicates matched just a single row. Practically it's probably most usable when combined with the PKs, but again, no tight coupling. All that said, the only way you can actually use IS_DELETED (without resorting to raw RPCs) is by using the diff scan private API, which mandates an ORDERED scan, meaning the server will add PK columns if the client omitted them. http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@1533 PS4, Line 1533: if (!GetRequiredColumnPrivilegesForNewScan(scan_pb, schema, &required_column_privileges)) { > Not sure I follow. It's not just the projected columns -- it's the projecte I see your point re: not just being projected columns. Yeah, my issue is that the variable and function naming imply that the nouns in play are "privileges", but they're not; they're column IDs. This leads to some confusion on L1534 where you're iterating on a collection of "privileges" from which you're extracting "column IDs". If you can't find another way to name this don't worry about it; it's not a big deal. http://gerrit.cloudera.org:8080/#/c/11753/7/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11753/7/src/kudu/tserver/tablet_service.cc@409 PS7, Line 409: vector<ColumnId> cols = schema.column_ids(); : *required_column_privileges = unordered_set<ColumnId>(cols.begin(), cols.end()); If column_ids() returns a cref, you could combine these two lines without losing efficiency. -- To view, visit http://gerrit.cloudera.org:8080/11753 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7a5d81cf215a5d936f8853feba05778038764905 Gerrit-Change-Number: 11753 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 18 Mar 2019 04:21:19 +0000 Gerrit-HasComments: Yes
