Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12122 )
Change subject: KUDU-2543 pt 2: pass around default authz tokens ...................................................................... Patch Set 10: (11 comments) http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h File src/kudu/client/authz_token_cache.h: http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@123 PS10, Line 123: table id nit: either 'table ID' or change 'table ID' --> 'table id' at line 130. http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc File src/kudu/client/authz_token_cache.cc: http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@129 PS10, Line 129: RpcAndCallbacks({ rpc, { std::move(callback) } }) > nit: could you get away with just { rpc, { std::move(callback) } } ? whoops, apparently you cannot but you could get away with RpcAndCallbacks(rpc, { std::move(callback) }) http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@143 PS10, Line 143: rpc_and_cbs.second > I'm not sure .second is necessary here: the EraseKeyReturnValuePtr() is sup I take this back: I missed the fact the value in the map is std::pair. http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client-test.cc@5833 PS10, Line 5833: ASSERT_TRUE(new_data->FetchCachedAuthzToken(table_id, &cached_token)); Does it make sense to verify that storing authz token into 'new_data' does not affect token stored via 'data'? I.e., fetch current token from 'data' and verify that's still 'new_token'? http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client.h@637 PS10, Line 637: FRIEND_TEST(ClientTest, TestInvalidAuthorizationTokens); Is it still around? For some reason, I could find this test in this changelist. Am I missing something? http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc File src/kudu/integration-tests/authz_token-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@143 PS10, Line 143: const char* kTableName = "test-table"; : const char* kUser = "token-user"; : const char* kBadUser = "bad-token-user"; nit: 'const char* x const' protects the public pointer member 'x' itself. http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@207 PS10, Line 207: int nit: the TotalCount() method of the metric returns uint64_t, why not to use this type for the return type of this function as well? http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@248 PS10, Line 248: with drop http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@484 PS10, Line 484: keep : // failing are always invalid ? http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@540 PS10, Line 540: shared_ptr<KuduSession> session(client_->NewSession()); Is this needed? http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master-test.cc@1771 PS10, Line 1771: ::testing::Values(true, false) nit: could use ::testing::Bool() instead -- To view, visit http://gerrit.cloudera.org:8080/12122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 Gerrit-Change-Number: 12122 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 11 Feb 2019 08:06:25 +0000 Gerrit-HasComments: Yes
