Alexey Serbin has posted comments on this change. ( )

Change subject: KUDU-2543 pt 2: pass around default authz tokens

Patch Set 10:

File src/kudu/client/authz_token_cache.h:
PS10, Line 123: table id
nit: either 'table ID' or change 'table ID' --> 'table id' at line 130.
File src/kudu/client/
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) })
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.
File src/kudu/client/
PS10, Line 5833:   ASSERT_TRUE(new_data->FetchCachedAuthzToken(table_id, 
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'?
File src/kudu/client/client.h:
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?
File src/kudu/integration-tests/
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.
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?
PS10, Line 248: with
PS10, Line 484: keep
              :   // failing
are always invalid ?
PS10, Line 540:   shared_ptr<KuduSession> session(client_->NewSession());
Is this needed?
File src/kudu/master/
PS10, Line 1771: ::testing::Values(true, false)
nit: could use ::testing::Bool() instead

To view, visit
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Andrew Wong <>
Gerrit-Reviewer: Hao Hao <>
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