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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <hao....@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 11 Feb 2019 08:06:25 +0000
Gerrit-HasComments: Yes

Reply via email to